Closed Bug 851788 Opened 12 years ago Closed 12 years ago

crash in JSScript::clearTraps

Categories

(Core :: JavaScript Engine, defect)

20 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 + verified
firefox21 + verified
firefox22 --- verified
firefox23 --- verified

People

(Reporter: scoobidiver, Assigned: till)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(2 files)

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
Crash Signature: [@ JSScript::clearTraps(js::FreeOp*)] → [@ JSScript::clearTraps(js::FreeOp*)] [@ JSScript::debugScript()]
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.
Blocks: 784294
Assignee: general → tschneidereit
This is fixed in all versions now. The patch in bug 787927 was uplifted to Aurora.
Assignee: tschneidereit → general
Keywords: qawanted
Assignee: general → tschneidereit
Till, what is needed from QA here? Do we have steps to reproduce this crash to verify it's fixed?
Flags: needinfo?(tschneidereit)
(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.
(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.
Flags: needinfo?(tschneidereit)
Keywords: qawanted
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.
Attachment #740812 - Flags: review?(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'.
Attachment #740812 - Flags: review?(jimb) → review+
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
Attachment #740812 - Flags: approval-mozilla-beta?
Attachment #740812 - Flags: approval-mozilla-aurora?
> 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
(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 ?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(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.
(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?
Attachment #740812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needinfo on :till for comment# 16
Flags: needinfo?(tschneidereit)
It's not fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #740812 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
This backs out all the self-hosted code that's active in Beta.
Attachment #743285 - Flags: review?(bbajaj)
Flags: needinfo?(tschneidereit)
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 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.
Attachment #743285 - Flags: review?(bbajaj) → review?(jwalden+bmo)
(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).
Attachment #743285 - Flags: review?(jwalden+bmo) → review+
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
Attachment #743285 - Flags: approval-mozilla-beta?
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.
Attachment #743285 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
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?
Depends on: 868369
No longer depends on: 868369
Is it fixed in all channels by the patch of bug 868369?
Flags: needinfo?(tschneidereit)
(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.
Flags: needinfo?(tschneidereit)
There have been no crashes since 23.0a1/20130508, 22.0a2/20130507, and 21.0b6 after the fix of bug 868369.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Marking this verified since this hasn't seemed to resurface.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: