Last Comment Bug 650753 - Crash [@ js::gc::Cell::compartment] with WeakMap, forced GC
: Crash [@ js::gc::Cell::compartment] with WeakMap, forced GC
Status: RESOLVED FIXED
fixed-in-tracemonkey
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: ---
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks: WeakMap
  Show dependency treegraph
 
Reported: 2011-04-18 03:18 PDT by Jesse Ruderman
Modified: 2013-01-04 13:21 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (requires extension for GC) (crashes when loaded) (78 bytes, text/html)
2011-04-18 03:18 PDT, Jesse Ruderman
no flags Details
patch (1.06 KB, patch)
2011-04-18 12:45 PDT, Gregor Wagner [:gwagner]
gal: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-04-18 03:18:21 PDT
Created attachment 526693 [details]
testcase (requires extension for GC) (crashes when loaded)

Tested version: http://hg.mozilla.org/tracemonkey/rev/63a06fbd23e0

1. Install 'DOM Fuzz Lite' from
    https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Load the testcase.

Result: crash [@ js::gc::Cell::compartment]

I couldn't get it to crash without the extension, which is surprising because the forced GC is at the end of the testcase.  I'm curious what's going on here.
Comment 1 Andreas Gal :gal 2011-04-18 10:37:51 PDT
Interesting. Gregor?
Comment 2 Gregor Wagner [:gwagner] 2011-04-18 12:45:37 PDT
Created attachment 526790 [details] [diff] [review]
patch

null is a gcThing for historically reasons according to luke.
isMarkable is what we want.
Comment 3 Andreas Gal :gal 2011-04-18 14:55:44 PDT
Comment on attachment 526790 [details] [diff] [review]
patch

Doh. Thanks.
Comment 4 Gregor Wagner [:gwagner] 2011-04-18 15:11:06 PDT
http://hg.mozilla.org/tracemonkey/rev/59325b2ca38b
Comment 5 Andreas Gal :gal 2011-04-18 15:21:54 PDT
jorendorff, this is probably the crash we had during the aurora landing
Comment 6 Jason Orendorff [:jorendorff] 2011-04-18 15:48:41 PDT
I'm so glad this turned up sooner rather than later.

Though, this patch absolutely should not have gotten an r+ without a js test. Don't worry about it, I'll push one.
Comment 7 Jesse Ruderman 2011-04-18 20:33:46 PDT
> I'm so glad this turned up sooner rather than later.

I found this by adding special WeakMap stuff to my DOM fuzzer and running it on TM branch (which I don't normally do).

Just for you <3
Comment 8 Jesse Ruderman 2011-04-18 20:34:20 PDT
Why does this bug only happen with a forced GC, and not with normal GCs or shutdown GC?
Comment 9 Gregor Wagner [:gwagner] 2011-04-18 20:43:58 PDT
(In reply to comment #8)
> Why does this bug only happen with a forced GC, and not with normal GCs or
> shutdown GC?

This path is only executed when the key is alive. I guess the keys are already  unreachable if we wait for the normal GC.
Comment 10 Jesse Ruderman 2011-04-18 20:52:59 PDT
Interesting. So we might want to check in several testcases, with various combinations of keeping the key and the map alive.
Comment 11 Jason Orendorff [:jorendorff] 2011-04-19 20:43:20 PDT
Pushed: http://hg.mozilla.org/tracemonkey/rev/332284d2b284

Jesse: <3
Comment 12 Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 15:45:03 PDT
http://hg.mozilla.org/mozilla-central/rev/332284d2b284
Comment 13 Christian Holler (:decoder) 2013-01-04 13:21:25 PST
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-650753.js.

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