Last Comment Bug 653026 - GC hazard with NoSuchMethod
: GC hazard with NoSuchMethod
Status: RESOLVED FIXED
[sg:critical?][fixed-in-tracemonkey][...
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks: 584917
  Show dependency treegraph
 
Reported: 2011-04-26 18:10 PDT by Bill McCloskey (:billm)
Modified: 2011-08-12 09:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
wanted
unaffected


Attachments
testcase (148 bytes, text/plain)
2011-04-26 18:10 PDT, Bill McCloskey (:billm)
no flags Details
patch (3.23 KB, patch)
2011-04-27 10:31 PDT, Bill McCloskey (:billm)
jwalden+bmo: review+
dmandelin: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-04-26 18:10:04 PDT
Created attachment 528500 [details]
testcase

This script crashes in a shell with no jit options.
Comment 1 Bill McCloskey (:billm) 2011-04-27 10:31:34 PDT
Created attachment 528630 [details] [diff] [review]
patch

The basic problem is that the new object being allocated in js_OnUnknownMethod is the only thing that roots the method. This object is not scanned during a GC because it has a NULL shape, so it's treated as a newborn object. This was apparently sort of intentional, to ensure that these objects don't escape into user code. However, I think it's more important that we scan them.

I set the shape so that the object looks non-native, and then I added a trace hook to the class. I thought about making it a native object, but I think then we'd need a proto for it, and I didn't want to deal with that. Please correct me if I'm wrong, Jeff.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-27 15:34:49 PDT
Comment on attachment 528630 [details] [diff] [review]
patch

Sounds reasonable.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-19 14:03:24 PDT
Can this land now that it's reviewed n' all? Would this patch be safe for beta?
Comment 4 Bill McCloskey (:billm) 2011-05-19 14:05:38 PDT
Sorry for the delay. I meant to land it yesterday but I forgot. I'll make sure it gets into the beta.
Comment 5 Bill McCloskey (:billm) 2011-05-20 12:23:48 PDT
http://hg.mozilla.org/tracemonkey/rev/10ba4918f08e

I pushed without the test. That needs to go in later.
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:17:02 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/10ba4918f08e
Comment 7 David Mandelin [:dmandelin] 2011-05-24 18:04:41 PDT
http://hg.mozilla.org/mozilla-beta/rev/cb0b3bc9c53b
Comment 8 David Mandelin [:dmandelin] 2011-05-25 10:41:57 PDT
This made it to mozilla-aurora as

http://hg.mozilla.org/mozilla-aurora/rev/10ba4918f08e
Comment 9 Daniel Veditz [:dveditz] 2011-06-08 10:51:59 PDT
On the 1.9.2 branch I get "TypeError: obj.x(a()) is not a function" rather than a crash. Does that mean that branch is not affected by this bug? Do we know what regressed it?
Comment 10 Bill McCloskey (:billm) 2011-06-09 17:04:44 PDT
It looks like the testcase in the bug regressed in bug 584917, so 3.6 should be safe. Before that patch, we rooted the function to call via the NoSuchMethod object's parent (or proto, I forget the argument order).

I'm still a bit worried about whether the other thing stored in the NoSuchMethod object in rooted. It can only be a jsid, but it seems like e4x might somehow allow an object to be stashed there. I don't really understand the code too well though. It would be good if someone who understands e4x better could take a look. I'm guessing this means Brendan or Igor?
Comment 11 Daniel Veditz [:dveditz] 2011-06-09 18:12:53 PDT
Thanks, for now we'll not track this on the old branch.
Comment 12 Virgil Dicu [:virgil] [QA] 2011-08-12 09:09:38 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0

Could you please provide some clear STR in order to verify this issue?

Note You need to log in before you can comment on or make changes to this bug.