Bug 158093 - Implement internals.observeGC to get called back when a Javascript object is GC'ed
Summary: Implement internals.observeGC to get called back when a Javascript object is ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117 154015
  Show dependency treegraph
 
Reported: 2016-05-25 14:46 PDT by Brady Eidson
Modified: 2016-05-26 11:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (19.79 KB, patch)
2016-05-25 17:24 PDT, Brady Eidson
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff
Patch for EWS/landing (18.41 KB, patch)
2016-05-25 21:28 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch for EWS (19.75 KB, patch)
2016-05-25 22:28 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch for EWS + landing (18.41 KB, patch)
2016-05-26 09:26 PDT, Brady Eidson
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.08 MB, application/zip)
2016-05-26 10:02 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-05-25 14:46:33 PDT
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.
Comment 3 Brady Eidson 2016-05-25 17:24:52 PDT
Created attachment 279842 [details]
Patch
Comment 4 Geoffrey Garen 2016-05-25 17:36:40 PDT
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.
Comment 5 Brady Eidson 2016-05-25 18:49:06 PDT
(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.
Comment 6 Brady Eidson 2016-05-25 21:28:51 PDT
Created attachment 279864 [details]
Patch for EWS/landing
Comment 7 Brady Eidson 2016-05-25 21:33:52 PDT
Same linker error the first patch was seeing... Weiiiiiiird.
Comment 8 Brady Eidson 2016-05-25 21:34:32 PDT
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.
Comment 9 Brady Eidson 2016-05-25 22:28:40 PDT
Created attachment 279866 [details]
Patch for EWS

Attempt to fix the WebCoreTestSupport linker error with some JSC exports.
Comment 10 Brady Eidson 2016-05-25 22:47:24 PDT
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.
Comment 11 Brady Eidson 2016-05-26 09:24:05 PDT
I was missing JSCJSValueInlines.h
*slaps forehead*
Comment 12 Brady Eidson 2016-05-26 09:26:08 PDT
Created attachment 279891 [details]
Patch for EWS + landing
Comment 13 Build Bot 2016-05-26 10:02:11 PDT
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.
Comment 14 Build Bot 2016-05-26 10:02:14 PDT
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
Comment 15 Brady Eidson 2016-05-26 10:06:42 PDT
(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.
Comment 16 Brady Eidson 2016-05-26 10:07:07 PDT
(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.
Comment 17 WebKit Commit Bot 2016-05-26 10:08:54 PDT
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
Comment 18 WebKit Commit Bot 2016-05-26 10:17:33 PDT
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
Comment 19 Brady Eidson 2016-05-26 10:21:36 PDT
(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
Comment 20 Brady Eidson 2016-05-26 10:22:11 PDT
Landed manually in http://trac.webkit.org/changeset/201422
Comment 21 Geoffrey Garen 2016-05-26 10:47:35 PDT
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.