Closed Bug 863766 Opened 7 years ago Closed 7 years ago

Crash in release builds when deleting a JSContext that has outstanding requests

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- verified
firefox23 --- verified
firefox-esr17 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

No good can come of this.
Comment on attachment 739645 [details] [diff] [review]
I like to crash it crash it

Android/B2G oranges are from another patch that was backed out.
  https://tbpl.mozilla.org/?tree=Try&rev=40f6ebae28ae

This patch may not be a good idea, I don't know.  It depends on how much you trust outstandingRequests.  I could also wait until bholley lands more of his context stack cleanup, in which case our story here could be cleaner.
Attachment #739645 - Flags: review?(luke)
Attachment #739645 - Flags: feedback?(bobbyholley+bmo)
Maybe the THREADSAFE stuff still needs to be there, come to think of it...
Comment on attachment 739645 [details] [diff] [review]
I like to crash it crash it

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

f=bholley with the JS_THREADSAFE stuff back in. We'll rip that out at some point in the future.
Attachment #739645 - Flags: feedback?(bobbyholley+bmo) → feedback+
Comment on attachment 739645 [details] [diff] [review]
I like to crash it crash it

You'll need to add back the #ifdef JS_THREADSAFE (see outstandingRequests in jscntxt.h).
Attachment #739645 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/8a4e38fbd3f7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 739645 [details] [diff] [review]
I like to crash it crash it

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: possible security problems
Testing completed (on m-c, etc.): it has been on m-c for about a week
Risk to taking this patch (and alternatives if risky): should be fairly low, if we hit this crash we're probably already in a bad state that is going to crash anyways.
String or IDL/UUID changes made by this patch: none
Attachment #739645 - Flags: approval-mozilla-esr17?
Attachment #739645 - Flags: approval-mozilla-beta?
Attachment #739645 - Flags: approval-mozilla-aurora?
Depends on: 865915
Comment on attachment 739645 [details] [diff] [review]
I like to crash it crash it

I guess we should figure out this regression before landing on branches.
Attachment #739645 - Flags: approval-mozilla-esr17?
Attachment #739645 - Flags: approval-mozilla-beta?
Attachment #739645 - Flags: approval-mozilla-aurora?
Comment on attachment 739645 [details] [diff] [review]
I like to crash it crash it

See comment 8.  The regression was actually this patch detecting a bug in recently landed code.
Attachment #739645 - Flags: approval-mozilla-esr17?
Attachment #739645 - Flags: approval-mozilla-beta?
Attachment #739645 - Flags: approval-mozilla-aurora?
I guess at this point it is not a big deal if this doesn't get on beta.
(In reply to Andrew McCreight [:mccr8] from comment #12)
> I guess at this point it is not a big deal if this doesn't get on beta.

Yes & given we are in our final beta's and this may impact known/unknown signature volume on beta, lets just land it on aurora at this point .
Attachment #739645 - Flags: approval-mozilla-beta?
Attachment #739645 - Flags: approval-mozilla-beta-
Attachment #739645 - Flags: approval-mozilla-aurora?
Attachment #739645 - Flags: approval-mozilla-aurora+
Attachment #739645 - Flags: approval-mozilla-esr17?
Depends on: 868377
Test case or steps to reproduce crash?
Andrew, I don't think we can verify this bug as fixed until the related bug (whose bug file we're using to test this one) gets fixed correctly. This is for 17.0.7esr only. Just FYI.

Otherwise, verified fixed on builds FF22/23 2013-06-14.
You need to log in before you can comment on or make changes to this bug.