851788 - crash in JSScript::clearTraps

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Scoobidiver (away)]

It's currently #2 top crasher on Mac OS X and #1 on Linux in 20.0b5.
It first showed up in 22.0a1/20130314, 21.0a2/20130314 and 20.0b5. The regression windows are:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c1a5c44ae3d8&tochange=b672877ed046
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=2ba5a4a724ae&tochange=a64ef5e8030c
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=a853b233420d&tochange=163304f85fc1
It's likely a regression from bug 787927.

Signature 	JSScript::clearTraps(js::FreeOp*) More Reports Search
UUID	51af07fc-99ab-4b5e-b031-32e972130315
Date Processed	2013-03-15 07:10:48
Uptime	4476
Last Crash	1.4 weeks before submission
Install Age	1.2 hours since version was first installed.
Install Time	2013-03-15 05:38:52
Product	Firefox
Version	22.0a1
Build ID	20130314030914
Release Channel	nightly
OS	Mac OS X
OS Version	10.8.2 12C3012
Build Architecture	amd64
Build Architecture Info	family 6 model 58 stepping 9
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0xffffffffa1c76fea
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x 166GL Context? GL Context+ GL Layers? GL Layers+ 
Processor Notes 	sp-processor08.phx1.mozilla.com_25864:2008; exploitablity tool: ERROR: unable to analyze dump
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x 166

Frame 	Module 	Signature 	Source
0 	XUL 	JSScript::clearTraps 	js/src/jsscript.h:904
1 	XUL 	jsd_ClearAllExecutionHooks 	js/jsd/jsd_scpt.cpp:921
2 	XUL 	jsdService::ClearAllBreakpoints 	js/jsd/jsd_xpc.cpp:3020
3 	XUL 	jsdService::DeactivateDebugger 	js/jsd/jsd_xpc.cpp:2530
4 	XUL 	jsdService::Off 	js/jsd/jsd_xpc.cpp:2639
5 	XUL 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
6 	libnspr4.dylib 	PR_GetCurrentThread 	ptthread.c:621
7 	XUL 	nsThreadManager::GetIsMainThread 	nsThreadManager.cpp:272
8 	XUL 	NS_IsMainThread_P 	nsThreadUtils.cpp:137
9 	XUL 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:3085
10 	XUL 	js::DefineNativeProperty 	js/src/jsobj.cpp:3337
11 	XUL 	js::NewFunction 	js/src/jsobjinlines.h:1650
12 	XUL 	XUL@0xbd5660 	
13 	XUL 	JS_EvaluateUCScriptForPrincipalsVersionOrigin 	js/src/jsapi.cpp:5604
14 	XUL 	js::baseops::DefineGeneric 	js/src/jsobj.cpp:3120
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=JSScript%3A%3AclearTraps%28js%3A%3AFreeOp*%29

Comment 1

[Scoobidiver (away)]

More reports also at:
https://crash-stats.mozilla.com/report/list?signature=JSScript%3A%3AdebugScript%28%29

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

We're going to backout bug 784294 from FF20 and see if that reduces the volume here - but Till will keep working on the regression in FF21 (bug 844406) to see if a forward fix can be found in time for that release.

Comment 3

[Scoobidiver (away)]

Fixed in 20.0b6 by the backout of bug 784294:
https://hg.mozilla.org/releases/mozilla-beta/rev/56833fe7db8e

Comment 4

[Till Schneidereit [:till]]

This is fixed in all versions now. The patch in bug 787927 was uplifted to Aurora.

Comment 5

[u279076]

Till, what is needed from QA here? Do we have steps to reproduce this crash to verify it's fixed?

Comment 6

[Scoobidiver (away)]

(In reply to Till Schneidereit [:till] from comment #4)
> This is fixed in all versions now. The patch in bug 787927 was uplifted to
> Aurora.
I don't see any Aurora landing recently in bug 787937. In addition, crashes happen in 21.0b2, 22.0a2/20130410 and 23.0a1/20130409.

Comment 7

[Till Schneidereit [:till]]

(In reply to Scoobidiver from comment #6)
> (In reply to Till Schneidereit [:till] from comment #4)
> > This is fixed in all versions now. The patch in bug 787927 was uplifted to
> > Aurora.
> I don't see any Aurora landing recently in bug 787937. In addition, crashes
> happen in 21.0b2, 22.0a2/20130410 and 23.0a1/20130409.

Mmmh, seems like I have to investigate some more, then. Sorry for the noise.

Comment 8

[Till Schneidereit [:till]]

Attachment: prevent jsd_SetExecutionHook from operating on self-hosted functions.

AFAICT, this should fix the issue. At least, I can't reproduce using Firebug, which has been installed in all cases where the issue occurred.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 740812 [details] [diff] [review]
prevent jsd_SetExecutionHook from operating on self-hosted functions.

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

::: js/jsd/jsd_scpt.cpp
@@ +825,5 @@
>      JSBool rv;
>      JSCompartment* oldCompartment;
>  
>      JSD_LOCK();
> +    if ( JS_GetScriptIsSelfHosted(jsdscript->script) )

Pre-existing, but it would be nice to have a blank line between the JSD_LOCK() and the 'if'.

Comment 10

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27478e07af9d

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 740812 [details] [diff] [review]
prevent jsd_SetExecutionHook from operating on self-hosted functions.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 787927
User impact if declined: Increased crash rate when using Firebug
Testing completed (on m-c, etc.): landed on m-c, but not yet in Nightly
Risk to taking this patch (and alternatives if risky): n/a
String or IDL/UUID changes made by this patch: none

Comment 12

[Till Schneidereit [:till]]

> Testing completed (on m-c, etc.): landed on m-c, but not yet in Nightly

Er, for now it's only landed on m-i

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to Till Schneidereit [:till] from comment #11)
> Comment on attachment 740812 [details] [diff] [review]
> prevent jsd_SetExecutionHook from operating on self-hosted functions.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 787927
> User impact if declined: Increased crash rate when using Firebug
> Testing completed (on m-c, etc.): landed on m-c, but not yet in Nightly
> Risk to taking this patch (and alternatives if risky): n/a

:Till,Can you please give more information on the risk here ? If this patch carries risk then can we back out bug 784294 which we did for Fx20 which may be a better alternative to mitigate risk here given we only have our final two beta's left for Fx21 ?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27478e07af9d

Comment 15

[Till Schneidereit [:till]]

(In reply to bhavana bajaj [:bajaj] from comment #13)
> (In reply to Till Schneidereit [:till] from comment #11)
> > Comment on attachment 740812 [details] [diff] [review]
> > prevent jsd_SetExecutionHook from operating on self-hosted functions.
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): bug 787927
> > User impact if declined: Increased crash rate when using Firebug
> > Testing completed (on m-c, etc.): landed on m-c, but not yet in Nightly
> > Risk to taking this patch (and alternatives if risky): n/a
> 
> :Till,Can you please give more information on the risk here ? If this patch
> carries risk then can we back out bug 784294 which we did for Fx20 which may
> be a better alternative to mitigate risk here given we only have our final
> two beta's left for Fx21 ?

I can see how "n/a" wasn't a very good answer here, sorry.

I think the risk is virtually non-existent here as the patch basically causes self-hosted code to be ignored at a place where they should be ignored, but I could, of course, be wrong about that.

And yes, you're right: the alternative would be to back out all self-hosted code, as for Fx20.

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to Till Schneidereit [:till] from comment #15)
> (In reply to bhavana bajaj [:bajaj] from comment #13)
> > (In reply to Till Schneidereit [:till] from comment #11)
> > > Comment on attachment 740812 [details] [diff] [review]
> > > prevent jsd_SetExecutionHook from operating on self-hosted functions.
> > > 
> > > [Approval Request Comment]
> > > Bug caused by (feature/regressing bug #): bug 787927
> > > User impact if declined: Increased crash rate when using Firebug
> > > Testing completed (on m-c, etc.): landed on m-c, but not yet in Nightly
> > > Risk to taking this patch (and alternatives if risky): n/a
> > 
> > :Till,Can you please give more information on the risk here ? If this patch
> > carries risk then can we back out bug 784294 which we did for Fx20 which may
> > be a better alternative to mitigate risk here given we only have our final
> > two beta's left for Fx21 ?
> 
> I can see how "n/a" wasn't a very good answer here, sorry.
> 
> I think the risk is virtually non-existent here as the patch basically
> causes self-hosted code to be ignored at a place where they should be
> ignored, but I could, of course, be wrong about that.
> 
> And yes, you're right: the alternative would be to back out all self-hosted
> code, as for Fx20.

Based on the above comment, I am comfortable going for the forward fix in this bug on aurora but for Fx 21 Beta I prefer a backout of bug 784294 and maintain the status-quo for one more cycle since we are in our final two beta's and it may be difficult to identify fallout's before we ship FX21. Please let me know if there if there are major concerns in backing out self-hosted code vs taking the risk of regressing something new here?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/978b849fd702

Comment 18

[bhavana bajaj [:bajaj]]

needinfo on :till for comment# 16

Comment 19

[Scoobidiver (away)]

It's not fixed.

Comment 20

[bhavana bajaj [:bajaj]]

Comment on attachment 740812 [details] [diff] [review]
prevent jsd_SetExecutionHook from operating on self-hosted functions.

a- on the fwd fix for beta, we are expecting incoming of a backout patch to help here.

Comment 21

[Till Schneidereit [:till]]

Attachment: ackout of self-hosted Array extras to fix crashes. r=

This backs out all the self-hosted code that's active in Beta.

Comment 22

[Steve Fink [:sfink] [:s:]]

till: looks like it's finding some stale JSScript* that you don't want it to.

JSD will observe a script if the interruptHook fires, or if you step into a function, or you throw an exception while running the script. But it looks to me like it should only see things that JSBrokenFrameIterator would see, which only include what NonBuiltinScriptFrameIter can see. Should that already be suppressing your self-hosted scripts? (I don't know what builtin means here.)

I would recommend looking at one of the minidumps for this crash and going to the jsd_ClearAllExecutionHooksForScript frame and looking at the contents of jsdscript. (I think it should be valid, even though the corresponding JSScript* may point to junk.) That should give you the filename and base line number.

jsdscript will be a JSDScript. You can pull a jsdIScript out of that with JSD_GetScriptPrivate() and casting.

Comment 23

[bhavana bajaj [:bajaj]]

Comment on attachment 743285 [details] [diff] [review]
ackout of self-hosted Array extras to fix crashes. r=

Till, not the best person to review as I have no clue about this code.
I think you would need to request review from someone else here.

Passing it onto :Waldo as of now as he may be familiar with the code due to his comments in 784294 :) feel free to change the reviewer as needed.

Comment 24

[Till Schneidereit [:till]]

(In reply to bhavana bajaj [:bajaj] from comment #23)
> Till, not the best person to review as I have no clue about this code.
> I think you would need to request review from someone else here.

Right, sorry; don't know what I was thinking.

> 
> Passing it onto :Waldo as of now as he may be familiar with the code due to
> his comments in 784294 :) feel free to change the reviewer as needed.

@Waldo: this is about as clear-cut as it gets and has been done in identical fashion for the last beta (he said, with tears in his eyes).

Comment 25

[Till Schneidereit [:till]]

Comment on attachment 743285 [details] [diff] [review]
ackout of self-hosted Array extras to fix crashes. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 787927
User impact if declined: Increased crash rate when using Firebug
Testing completed (on m-c, etc.): This is a straight-forward backout, so, no.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 743285 [details] [diff] [review]
ackout of self-hosted Array extras to fix crashes. r=

Low risk backout needed on beta to avoid a top-crasher.Request to land it asap.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/11275b673611

Fixed for Fx21 by backout. Marking bug 787927 as disabled.

Comment 28

[Jeff Walden [:Waldo]]

Backed out the backout.  (No, you did *not* hear that I like backouts.  :-P )

https://hg.mozilla.org/releases/mozilla-beta/rev/6dfc0800664b

There was a subsequent API change in bug 839420 that made the backout patch not actually build, turns out.  After the backout of the backout, I redid the backout, made the changes to work with bug 839420's changes, and relanded the backout:

https://hg.mozilla.org/releases/mozilla-beta/rev/d2da3bca4656

Backout.

Comment 29

[u279076]

Is there anything specific QA can check to confirm the backout succeeded? It does not appear as though this crash was ever reproducible internally so will we just speculate based on crashstats?

Comment 30

[Scoobidiver (away)]

Is it fixed in all channels by the patch of bug 868369?

Comment 31

[Till Schneidereit [:till]]

(In reply to Scoobidiver from comment #30)
> Is it fixed in all channels by the patch of bug 868369?

That is the hope, yes. As I wasn't able to reproduce the crash, I'll have to wait for a few days to be sure. Based on a few days of theoretical analysis of the crash's potential causes, however, I'm pretty confident in the fix.

Comment 32

[Scoobidiver (away)]

There have been no crashes since 23.0a1/20130508, 22.0a2/20130507, and 21.0b6 after the fix of bug 868369.

Comment 33

[u279076]

Marking this verified since this hasn't seemed to resurface.