[Webkit-unassigned] [Bug 237060] Queue throwing exception of blob objects during destruction

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=237060

--- Comment #11 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 452987
  --> https://bugs.webkit.org/attachment.cgi?id=452987
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452987&action=review

> Source/JavaScriptCore/runtime/VM.cpp:422
> +    setShuttingDown(true);

setIsShuttingDown()

> Source/JavaScriptCore/runtime/VM.h:292
> +    bool shuttingDown() const { return m_shuttingDown; }

Per WebKit coding style, booleans need to start with a prefix, I think this should be:
bool isShuttingDown() const { return m_isShuttingDown; }

> Source/JavaScriptCore/runtime/VM.h:293
> +    void setShuttingDown(bool value) { m_shuttingDown = value; } 

I don't think we need the boolean parameter, this could be setIsShuttingDown() or markAsShuttingDown(). Once you're shutting down, I don't think there is any coming back to running, is there?

> Source/WebCore/fileapi/Blob.cpp:290
> +            if (!vm.shuttingDown()) {

In WebKit, we prefer early returns than nesting. This should be:
if (vm.shuttingDown())
    return;

But also, I don't see how this is scalable. There are tons of functions in WebCore that end up running JS (by throwing exceptions or firing events). How is this didFail() function any different? Surely, we cannot expect EVERY dom function to check this flag. We would need a better choke point. Maybe in the bindings function that actually ends up firing the event or throwing the exception? There should really not be much JSC logic inside WebCore/DOM implementation.

>> Source/WebCore/fileapi/Blob.cpp:292
>> +                context->eventLoop().queueTask(TaskSource::InternalAsyncTask, [this, protectedThis = Ref { *this }, code] {
> 
> The fact that we're needing to ref `this` here feels wrong because we're calling this from the destructor.  Why do we need to move the exception into m_exception if the object is already being destructed, granted, that didFail() may be called from places not in the destruction path.  This hints that this Blob code is itself not aware of what it's doing.  Let's get input from a WebCore engineer on this to determine if this is actually correct to do.

Blob is an ActiveDOMObject, we should probably call queueTaskKeepingObjectAlive() from ActiveDOMObject base class?

queueTaskKeepingObjectAlive(*this, TaskSource::InternalAsyncTask, [this, code] {
  // ...
});

Note that you wouldn't need to capture protectedThis then since queueTaskKeepingObjectAlive does this for you (in addition to keeping the JS wrapper alive).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220223/8230ac78/attachment.htm>