Bug 102328 – MutationObserver wrapper should not be collected while still observing

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

MutationObserver wrapper should not be collected while still observing

[Elliott Sprehn]

Created attachment 174338 [details]
Patch

[Early Warning System Bot]

Comment on attachment 174338 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 174338 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 174338 [details]
Patch

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

[Elliott Sprehn]

Created attachment 174350 [details]
Patch

Woops, deleted too many lines from the JSC code.

[Adam Barth]

Comment on attachment 174350 [details]
Patch

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

> Source/WebCore/dom/MutationObserver.cpp:60
> -PassRefPtr<MutationObserver> MutationObserver::create(PassRefPtr<MutationCallback> callback)
> +PassRefPtr<MutationObserver> MutationObserver::create(PassRefPtr<MutationCallback> callback, ScriptExecutionContext* context)

Don't we usually pass the ScriptExecutionContext as the first argument?

[Elliott Sprehn]

(In reply to comment #6)
> (From update of attachment 174350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174350&action=review
> 
> > Source/WebCore/dom/MutationObserver.cpp:60
> > -PassRefPtr<MutationObserver> MutationObserver::create(PassRefPtr<MutationCallback> callback)
> > +PassRefPtr<MutationObserver> MutationObserver::create(PassRefPtr<MutationCallback> callback, ScriptExecutionContext* context)
> 
> Don't we usually pass the ScriptExecutionContext as the first argument?

Yeah we've talked about this before. The code generator passes it as the last one (ex. V8MutationCallback::create), but humans pass it first like XMLHttpRequest. We should fix the code generator so code isn't weird like in V8MutationObserverCustom having two create() calls with a different order.

I'll change this to match XMLHttpRequest.

[Elliott Sprehn]

Created attachment 174483 [details]
Patch for landing

[Adam Barth]

Comment on attachment 174483 [details]
Patch for landing

Silly code generator.  Trix are for kids.

[WebKit Review Bot]

Comment on attachment 174483 [details]
Patch for landing

Rejecting attachment 174483 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 157 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 157.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14843700

[WebKit Review Bot]

Comment on attachment 174483 [details]
Patch for landing

Clearing flags on attachment: 174483

Committed r134830: <http://trac.webkit.org/changeset/134830>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102701

[Adam Klein]

In offline discussion, abarth suggested an alternate approach using opaque roots: put each MutationObserver wrapper in the same object group(s) as the Nodes it's observing.

[Adam Barth]

In particular, we can't use ActiveDOMObject because MutationObservers need to work with Frameless documents, like those created by XMLHttpRequest.

[Adam Klein]

I tried to do this with implicit references and ran into flakiness. Opaque roots seem to work great, patch coming...

[Adam Klein]

Created attachment 175083 [details]
Patch

[Elliott Sprehn]

Comment on attachment 175083 [details]
Patch

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

Nice!

> Source/WebCore/dom/MutationObserverRegistration.cpp:130
> +    if (m_transientRegistrationNodes) {

Early return is usually nicer than indenting everything.

[Adam Barth]

Comment on attachment 175083 [details]
Patch

This looks good.  Do we need to skip these tests for JSC while we work on a fix for the JSC bindings?

[Adam Klein]

Created attachment 175088 [details]
Patch for landing

[Adam Klein]

(In reply to comment #19)
> (From update of attachment 175083 [details])
> This looks good.  Do we need to skip these tests for JSC while we work on a fix for the JSC bindings?

Skipped in all non-chromium ports with a pointer to bug 102744.

[WebKit Review Bot]

Comment on attachment 175088 [details]
Patch for landing

Clearing flags on attachment: 175088

Committed r135228: <http://trac.webkit.org/changeset/135228>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.