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.
Created attachment 180827 [details] Patch
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 on attachment 180827 [details] Patch Attachment 180827 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15559492
Comment on attachment 180827 [details] Patch Attachment 180827 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15546638
Comment on attachment 180827 [details] Patch Attachment 180827 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15543642
Comment on attachment 180827 [details] Patch Attachment 180827 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15572185
Comment on attachment 180827 [details] Patch Attachment 180827 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15543644
Comment on attachment 180827 [details] Patch Attachment 180827 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15656222
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.
Created attachment 181201 [details] Patch
Comment on attachment 181201 [details] Patch Attachment 181201 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15665185
Comment on attachment 181201 [details] Patch Attachment 181201 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15630998
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.
Created attachment 181205 [details] Patch
Comment on attachment 181205 [details] Patch Attachment 181205 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15671007
Comment on attachment 181205 [details] Patch Attachment 181205 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15671006
Comment on attachment 181205 [details] Patch Attachment 181205 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15670010
Comment on attachment 181205 [details] Patch Attachment 181205 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15669016
Comment on attachment 181205 [details] Patch Attachment 181205 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15647805
Created attachment 181210 [details] Patch
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.
(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 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 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 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 on attachment 181210 [details] Patch Clearing flags on attachment: 181210 Committed r138754: <http://trac.webkit.org/changeset/138754>
All reviewed patches have been landed. Closing bug.
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
(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