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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

No good can come of this.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: I like to crash it crash it

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

Maybe the THREADSAFE stuff still needs to be there, come to think of it...

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Luke Wagner [:luke]]

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).

Comment 6

[Andrew McCreight [:mccr8]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4e38fbd3f7

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a4e38fbd3f7

Comment 8

[Andrew McCreight [:mccr8]]

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

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Andrew McCreight [:mccr8]]

I guess at this point it is not a big deal if this doesn't get on beta.

Comment 13

[bhavana bajaj [:bajaj]]

(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 .

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bcbc112ad19

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/2837cdf63d0b

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Test case or steps to reproduce crash?

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

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.