Bug 105810 – [Mutation Observers] prevent delivery while recipient context is suspended

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Rafael Weinstein]

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.

[Rafael Weinstein]

Created attachment 180827 [details]
Patch

[Rafael Weinstein]

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.

[Early Warning System Bot]

Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15559492

[Early Warning System Bot]

Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15546638

[EFL EWS Bot]

Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15543642

[Build Bot]

Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15572185

[kov's GTK+ EWS bot]

Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15543644

[Build Bot]

Comment on attachment 180827 [details]
Patch

Attachment 180827 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15656222

[Adam Barth]

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.

[Rafael Weinstein]

Created attachment 181201 [details]
Patch

[Early Warning System Bot]

Comment on attachment 181201 [details]
Patch

Attachment 181201 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15665185

[Early Warning System Bot]

Comment on attachment 181201 [details]
Patch

Attachment 181201 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15630998

[Elliott Sprehn]

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.

[Rafael Weinstein]

Created attachment 181205 [details]
Patch

[Early Warning System Bot]

Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15671007

[Early Warning System Bot]

Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15671006

[EFL EWS Bot]

Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15670010

[Build Bot]

Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15669016

[Build Bot]

Comment on attachment 181205 [details]
Patch

Attachment 181205 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15647805

[Rafael Weinstein]

Created attachment 181210 [details]
Patch

[Rafael Weinstein]

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.

[Elliott Sprehn]

(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.

[Adam Barth]

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;" ?

[Rafael Weinstein]

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.

[Rafael Weinstein]

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.

[WebKit Review Bot]

Comment on attachment 181210 [details]
Patch

Clearing flags on attachment: 181210

Committed r138754: <http://trac.webkit.org/changeset/138754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch appears to have broken bindings tests:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/4437
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/4437/steps/bindings-generation-tests/logs/stdio

[Adam Klein]

(In reply to comment #28)
> This patch appears to have broken bindings tests:
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/4437
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/4437/steps/bindings-generation-tests/logs/stdio

Fixed in http://trac.webkit.org/changeset/138769