1247666 - Hazard analysis dies with TypeError: edge.PEdgeCallInstance is undefined

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

The patch in bug 1243808 causes the hazard analysis to die with the following error:

js/src/devtools/rootAnalysis/CFG.js:72:1 TypeError: edge.PEdgeCallInstance is undefined

https://treeherder.mozilla.org/logviewer.html#?job_id=21458492&repo=mozilla-inbound

Comment 1

[Steve Fink [:sfink] [:s:]]

jonco - sorry, I tried doing this as part of other hazard-related stuff I was working on today, and have not yet managed to reproduce. I really need to back off and just do a local hazard build with your patch. If neither build in https://treeherder.mozilla.org/#/jobs?repo=try&revision=96685e0844e0 ends up showing the problem, then I'll do that first thing tomorrow. (If those do show it, they should produce enough artifacts for me to debug the problem.)

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Do not require all functions to have a PEdgeCallInstance, and correctly test isSuppressConstructor

So... the problem is the code demanded this PEdgeCallInstance thing, which doesn't exist for non-methods. Which makes it really surprising that it ever worked, until I finally realized that the code should only run for calls to AutoSuppressGC-like constructors.

So, two bugs: (1) it's ok if we don't find PEdgeCallInstance, and (2) it shouldn't treat a function call as an AutoSuppressGC constructor simply because it takes an AutoSuppressGC parameter (or has the substring "::AutoSuppressGC" anywhere in its full function type name).

Sorry for the trouble.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8719090 [details] [diff] [review]
Do not require all functions to have a PEdgeCallInstance, and correctly test isSuppressConstructor

Review of attachment 8719090 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks for fixing this!

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) (Away until 22nd Feb?) from comment #3)
> Comment on attachment 8719090 [details] [diff] [review]
> Do not require all functions to have a PEdgeCallInstance, and correctly test
> isSuppressConstructor
> 
> Review of attachment 8719090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  Thanks for fixing this!

Sadly, it's not fixed. It turns out that fixing the isSuppressConstructor code makes it (correctly) only match constructors, whereas the original matched both constructors and destructors (because ~Foo turns out to contain the substring "::Foo", because js::gc::Foo::~foo), and apparently the code that uses it incorrectly depends on that. :-( I'll try to fix that today. As-is, you'll get a few thousand new hazards.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35d9e774bfee
https://hg.mozilla.org/integration/mozilla-inbound/rev/c767ef5dbe47

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/35d9e774bfee
https://hg.mozilla.org/mozilla-central/rev/c767ef5dbe47

Comment 7

[Steve Fink [:sfink] [:s:]]

clearing needinfo, things are good here