Bug 105810 - [Mutation Observers] prevent delivery while recipient context is suspended
Summary: [Mutation Observers] prevent delivery while recipient context is suspended
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2012-12-27 15:29 PST by Rafael Weinstein
Modified: 2013-01-03 17:19 PST (History)
22 users (show)

See Also:


Attachments
Patch (8.42 KB, patch)
2012-12-27 15:34 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2013-01-03 12:19 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (10.05 KB, patch)
2013-01-03 12:39 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (10.05 KB, patch)
2013-01-03 13:33 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2012-12-27 15:29:46 PST
If the execution context is paused, but triggers a mutation (e.g. by stepping through code), Mutation Observers should refrain from attempting delivery until the context is resumed.

Currently, the delivery just gets dropped on the floor.
Comment 1 Rafael Weinstein 2012-12-27 15:34:36 PST
Created attachment 180827 [details]
Patch
Comment 2 Rafael Weinstein 2012-12-27 15:36:42 PST
Pavel,

This is my first crack at an inspector test, please let me know if this is ok.

Adam/Ojan, I'm guessing the way I'm accessing the ActiveDOMCallback is wrong here. What's a better way to do this? It seems like maybe the MutationCallback should have a virtual canDeliver(), which JSC and V8 can implement respectively, but I'm not sure how to do that.

Once I get the two above sorted, I'll attempt to implement JSC.
Comment 3 Early Warning System Bot 2012-12-27 15:45:20 PST
Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15559492
Comment 4 Early Warning System Bot 2012-12-27 15:46:40 PST
Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15546638
Comment 5 EFL EWS Bot 2012-12-27 16:12:37 PST
Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15543642
Comment 6 Build Bot 2012-12-27 16:37:18 PST
Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15572185
Comment 7 kov's GTK+ EWS bot 2012-12-27 17:17:39 PST
Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15543644
Comment 8 Build Bot 2013-01-02 15:59:37 PST
Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15656222
Comment 9 Adam Barth 2013-01-03 11:27:03 PST
Comment on attachment 180827 [details]
Patch

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

> Source/WebCore/dom/MutationObserver.cpp:193
> +bool MutationObserver::canDeliver()
> +{
> +// FIXME: How does JSC suspend for inspection?
> +#if USE(V8)
> +    return reinterpret_cast<V8MutationCallback*>(m_callback.get())->canInvokeCallback();
> +#else
> +    return true;
> +#endif
> +}

This isn't right.  You mean to ask the callback's context whether active DOM objects are suspended.
Comment 10 Rafael Weinstein 2013-01-03 12:19:23 PST
Created attachment 181201 [details]
Patch
Comment 11 Early Warning System Bot 2013-01-03 12:30:45 PST
Comment on attachment 181201 [details]
Patch

Attachment 181201 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15665185
Comment 12 Early Warning System Bot 2013-01-03 12:31:53 PST
Comment on attachment 181201 [details]
Patch

Attachment 181201 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15630998
Comment 13 Elliott Sprehn 2013-01-03 12:36:04 PST
Comment on attachment 181201 [details]
Patch

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

> Source/WebCore/dom/MutationObserver.cpp:241
> +                suspendedMutationObservers().add(observers[i]);

Why not just call activeMutationObservers().add() and put the suspended ones back into the list? I don't think you need the two lists and all the copying logic above.
Comment 14 Rafael Weinstein 2013-01-03 12:39:38 PST
Created attachment 181205 [details]
Patch
Comment 15 Early Warning System Bot 2013-01-03 13:00:11 PST
Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15671007
Comment 16 Early Warning System Bot 2013-01-03 13:00:21 PST
Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15671006
Comment 17 EFL EWS Bot 2013-01-03 13:05:15 PST
Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15670010
Comment 18 Build Bot 2013-01-03 13:16:03 PST
Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15669016
Comment 19 Build Bot 2013-01-03 13:29:33 PST
Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15647805
Comment 20 Rafael Weinstein 2013-01-03 13:33:24 PST
Created attachment 181210 [details]
Patch
Comment 21 Rafael Weinstein 2013-01-03 13:39:01 PST
Comment on attachment 181201 [details]
Patch

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

>> Source/WebCore/dom/MutationObserver.cpp:241
>> +                suspendedMutationObservers().add(observers[i]);
> 
> Why not just call activeMutationObservers().add() and put the suspended ones back into the list? I don't think you need the two lists and all the copying logic above.

That would case this loop to non-terminate. deliverAllMutations will get called at the end of every task. By putting observers into the suspended list, it means that each task makes an attempt to redeliver to any pending. Notice that this loop is while(!activeMutationObservers().isEmpty()). We can't keep adding suspended back into it. Within this loop, nothing is going to cause them to become unsuspended.
Comment 22 Elliott Sprehn 2013-01-03 13:44:10 PST
(In reply to comment #21)
> (From update of attachment 181201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181201&action=review
> 
> >> Source/WebCore/dom/MutationObserver.cpp:241
> >> +                suspendedMutationObservers().add(observers[i]);
> > 
> > Why not just call activeMutationObservers().add() and put the suspended ones back into the list? I don't think you need the two lists and all the copying logic above.
> 
> That would case this loop to non-terminate. deliverAllMutations will get called at the end of every task. By putting observers into the suspended list, it means that each task makes an attempt to redeliver to any pending. Notice that this loop is while(!activeMutationObservers().isEmpty()). We can't keep adding suspended back into it. Within this loop, nothing is going to cause them to become unsuspended.

I see, could we just put a stack allocated set outside the loop and copy into that then? Right after the loop add them all back. That's like 3 lines of code instead of all this.
Comment 23 Adam Barth 2013-01-03 13:53:27 PST
Comment on attachment 181210 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2833
> +    push(@headerContent, "    virtual ScriptExecutionContext* scriptExecutionContext() const { return ContextDestructionObserver::scriptExecutionContext(); }\n\n");

Can't we just write:

"using ContextDestructionObserver::scriptExecutionContext;" ?
Comment 24 Rafael Weinstein 2013-01-03 14:01:36 PST
Comment on attachment 181201 [details]
Patch

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

>>>> Source/WebCore/dom/MutationObserver.cpp:241
>>>> +                suspendedMutationObservers().add(observers[i]);
>>> 
>>> Why not just call activeMutationObservers().add() and put the suspended ones back into the list? I don't think you need the two lists and all the copying logic above.
>> 
>> That would case this loop to non-terminate. deliverAllMutations will get called at the end of every task. By putting observers into the suspended list, it means that each task makes an attempt to redeliver to any pending. Notice that this loop is while(!activeMutationObservers().isEmpty()). We can't keep adding suspended back into it. Within this loop, nothing is going to cause them to become unsuspended.
> 
> I see, could we just put a stack allocated set outside the loop and copy into that then? Right after the loop add them all back. That's like 3 lines of code instead of all this.

No. Observers aren't going to move from suspended->ready within the same stack. They will change states with later calls from a run loop.
Comment 25 Rafael Weinstein 2013-01-03 14:54:24 PST
Comment on attachment 181210 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2833
>> +    push(@headerContent, "    virtual ScriptExecutionContext* scriptExecutionContext() const { return ContextDestructionObserver::scriptExecutionContext(); }\n\n");
> 
> Can't we just write:
> 
> "using ContextDestructionObserver::scriptExecutionContext;" ?

nope. that doesn't appear to work.
Comment 26 WebKit Review Bot 2013-01-03 15:24:04 PST
Comment on attachment 181210 [details]
Patch

Clearing flags on attachment: 181210

Committed r138754: <http://trac.webkit.org/changeset/138754>
Comment 27 WebKit Review Bot 2013-01-03 15:24:10 PST
All reviewed patches have been landed.  Closing bug.