Last Comment Bug 712480 - Assert in most JSAPI functions that we're not inside a GC
: Assert in most JSAPI functions that we're not inside a GC
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: [PTO to Dec5] Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 713069
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-20 15:48 PST by [PTO to Dec5] Bill McCloskey (:billm)
Modified: 2012-01-01 21:03 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (75.87 KB, patch)
2011-12-28 13:28 PST, [PTO to Dec5] Bill McCloskey (:billm)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description [PTO to Dec5] Bill McCloskey (:billm) 2011-12-20 15:48:18 PST
Bug 711794 is a case where a finalizer is calling a JSAPI function that can GC. We should be able to assert against this.

We would have to identify a subset of the API that is safe. Things like getting reserved slots probably should be allowed during finalization. But most things should not. It probably makes sense to add asserts on every JSAPI function and then remove them as needed to get a browser running.

Brian, do you think Sixgill could help here? One problem I can foresee is that it's not obvious from Ben Turner's patch in the bug that it completely eliminates the possibility of calling XPCOM release code--in particular if the runtime is null. However, if we could annotate certain choke points like this one as being okay, then maybe it would be practical.
Comment 1 [PTO to Dec5] Bill McCloskey (:billm) 2011-12-28 13:28:18 PST
Created attachment 584629 [details] [diff] [review]
patch

With Ben's patch applied, this is green on tryserver.
Comment 2 [PTO to Dec5] Bill McCloskey (:billm) 2011-12-29 14:19:34 PST
Comment on attachment 584629 [details] [diff] [review]
patch

Switching to Waldo for a quicker review.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-29 16:42:27 PST
Comment on attachment 584629 [details] [diff] [review]
patch

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

r=me if it passes try.  And even if it does, I expect fallout for awhile from this.  But it is a Good Thing even still.
Comment 4 [PTO to Dec5] Bill McCloskey (:billm) 2012-01-01 17:21:21 PST
Yeah, we'll have to keep a close watch on this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/edffd43801d9
Comment 5 Phil Ringnalda (:philor) 2012-01-01 21:03:34 PST
https://hg.mozilla.org/mozilla-central/rev/edffd43801d9

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