Bug 158093 – Implement internals.observeGC to get called back when a Javascript object is GC'ed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Implement internals.observeGC to get called back when a Javascript object is GC'ed

observeGC is something V8 had for layout tests to cover leaks, ref cycles, etc.

It will be very useful for us to have, as well.

[Brady Eidson]

Example of existing use:
http://trac.webkit.org/browser/trunk/LayoutTests/storage/indexeddb/resources/cursor-leak.js

[Brady Eidson]

Example of existing use:
http://trac.webkit.org/browser/trunk/LayoutTests/storage/indexeddb/resources/cursor-leak.js

[Brady Eidson]

Created attachment 279842 [details]
Patch

[Geoffrey Garen]

Comment on attachment 279842 [details]
Patch

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

r=me with a change suggestion

> Source/WebCore/testing/GCObservation.h:50
> +    class WeakOwner : public JSC::WeakHandleOwner {
> +        void finalize(JSC::Handle<JSC::Unknown>, void* context) final;
> +    };
> +    WeakOwner m_weakOwner;

I realized that you can get even simpler and delete this stuff. Weak<T> becomes falsy automatically after the value is GC'd, so you can delete the weak owner and wasCollected() can just return !m_observedValue. That's a lot nicer.

[Brady Eidson]

(In reply to comment #4)
> Comment on attachment 279842 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279842&action=review
> 
> r=me with a change suggestion
> 
> > Source/WebCore/testing/GCObservation.h:50
> > +    class WeakOwner : public JSC::WeakHandleOwner {
> > +        void finalize(JSC::Handle<JSC::Unknown>, void* context) final;
> > +    };
> > +    WeakOwner m_weakOwner;
> 
> I realized that you can get even simpler and delete this stuff. Weak<T>
> becomes falsy automatically after the value is GC'd, so you can delete the
> weak owner and wasCollected() can just return !m_observedValue. That's a lot
> nicer.

Right - With the callback function approach we discussed in person, this approach was necessary.

But for the bool check, it can be much simpler.

As an added bonus, I'll make sure it builds!

Thanks for your help on this.

[Brady Eidson]

Created attachment 279864 [details]
Patch for EWS/landing

[Brady Eidson]

Same linker error the first patch was seeing... Weiiiiiiird.

[Brady Eidson]

Why did Mac Debug build fine (found the JSC symbol) but nobody else did (couldn't find it)?

I'm definitely building debug locally... guess I'll kick off a release build.

[Brady Eidson]

Created attachment 279866 [details]
Patch for EWS

Attempt to fix the WebCoreTestSupport linker error with some JSC exports.

[Brady Eidson]

Okay, at this point I simply don't know why WebCoreTestSupport can't find these symbols while linking against JavaScriptCore, even though WebCore obviously does.

I also don't know why the debug build works fine, but release does not.

I'll need the help of somebody else more knowledgable about the build system, exports, libs, etc.

[Brady Eidson]

I was missing JSCJSValueInlines.h
*slaps forehead*

[Brady Eidson]

Created attachment 279891 [details]
Patch for EWS + landing

[Build Bot]

Comment on attachment 279891 [details]
Patch for EWS + landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279893 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Brady Eidson]

(In reply to comment #14)
> Created attachment 279893 [details]
> Archive of layout-test-results from ews103 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

These failures cannot be related to this patch.

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #14)
> > Created attachment 279893 [details]
> > Archive of layout-test-results from ews103 for mac-yosemite
> > 
> > The attached test failures were seen while running run-webkit-tests on the
> > mac-ews.
> > Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
> 
> These failures cannot be related to this patch.

Seems much more likely that the W3C web platform test server encountered a problem during the test run.

[WebKit Commit Bot]

Comment on attachment 279891 [details]
Patch for EWS + landing

Rejecting attachment 279891 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 279891, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Geoff Garen found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/1386722

[WebKit Commit Bot]

Comment on attachment 279891 [details]
Patch for EWS + landing

Rejecting attachment 279891 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 279891, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Geoff Garen found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/1386760

[Brady Eidson]

(In reply to comment #18)
> Geoff Garen found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not
> appear to be a valid reviewer according to contributors.json.

This kind of nonsense is why I hate the commit queue. :P

[Brady Eidson]

Landed manually in http://trac.webkit.org/changeset/201422

[Geoffrey Garen]

Usually the right fix for a linker bug like this is to #include JSCInlines.h. Debug builds don't see the failure because they don't do the inlining thing.