MutationObserver wrapper should not be collected while still observing
Created attachment 174338 [details] Patch
Comment on attachment 174338 [details] Patch Attachment 174338 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14834626
Comment on attachment 174338 [details] Patch Attachment 174338 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14847258
Comment on attachment 174338 [details] Patch Attachment 174338 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14831650
Created attachment 174350 [details] Patch Woops, deleted too many lines from the JSC code.
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?
(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.
Created attachment 174483 [details] Patch for landing
Comment on attachment 174483 [details] Patch for landing Silly code generator. Trix are for kids.
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
Comment on attachment 174483 [details] Patch for landing Clearing flags on attachment: 174483 Committed r134830: <http://trac.webkit.org/changeset/134830>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 102701
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.
In particular, we can't use ActiveDOMObject because MutationObservers need to work with Frameless documents, like those created by XMLHttpRequest.
I tried to do this with implicit references and ran into flakiness. Opaque roots seem to work great, patch coming...
Created attachment 175083 [details] Patch
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.
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?
Created attachment 175088 [details] Patch for landing
(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.
Comment on attachment 175088 [details] Patch for landing Clearing flags on attachment: 175088 Committed r135228: <http://trac.webkit.org/changeset/135228>