Closed
Bug 1247666
Opened 9 years ago
Closed 9 years ago
Hazard analysis dies with TypeError: edge.PEdgeCallInstance is undefined
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: jonco, Assigned: sfink)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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
Assignee | ||
Comment 1•9 years ago
|
||
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.)
Flags: needinfo?(sphink)
Assignee | ||
Comment 2•9 years ago
|
||
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.
Attachment #8719090 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•9 years ago
|
||
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!
Attachment #8719090 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(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 6•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/35d9e774bfee
https://hg.mozilla.org/mozilla-central/rev/c767ef5dbe47
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•