Closed Bug 895774 Opened 7 years ago Closed 7 years ago

Permaorange fx-team devtools/debugger/test/browser_dbg_blackboxing-02.js | application crashed [@ js::BoxNonStrictThis(JSContext*, JS::MutableHandle<JS::Value>, bool*)]

Categories

(Core :: JavaScript Engine, defect)

25 Branch
x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: cbook, Unassigned)

References

()

Details

(Keywords: crash, intermittent-failure)

Crash Data

Attachments

(1 file, 1 obsolete file)

Ubuntu VM 12.04 fx-team opt test mochitest-browser-chrome on 2013-07-18 20:20:15 PDT for push c98c47341be6

slave: tst-linux32-ec2-360

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-02.js | Exited with code 11 during test run

PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_blackboxing-02.js | application crashed [@ js::BoxNonStrictThis(JSContext*, JS::MutableHandle<JS::Value>, bool*)]
20:40:21     INFO -  Crash dump filename: /tmp/tmpHhFX68/minidumps/223dc7e6-240d-c2c9-3199061b-4d5b7df9.dmp
20:40:21     INFO -  Operating system: Linux
20:40:21     INFO -                    0.0.0 Linux 3.2.0-23-generic-pae #36-Ubuntu SMP Tue Apr 10 22:19:09 UTC 2012 i686
20:40:21     INFO -  CPU: x86
20:40:21     INFO -       GenuineIntel family 6 model 45 stepping 7
20:40:21     INFO -       1 CPU
20:40:21     INFO -  Crash reason:  SIGSEGV
20:40:21     INFO -  Crash address: 0x18
20:40:21     INFO -  Thread 0 (crashed)
20:40:21     INFO -   0  libxul.so!js::BoxNonStrictThis(JSContext*, JS::MutableHandle<JS::Value>, bool*) [Interpreter.cpp:c98c47341be6 : 119 + 0x3]
20:40:21     INFO -      eip = 0xb5db9d63   esp = 0xbfb21280   ebp = 0xbfb212a8   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0xbfb212d7   edi = 0xbfb212d8   eax = 0x00000000   ecx = 0xaea5e780
20:40:21     INFO -      edx = 0x9b52d980   efl = 0x00010246
20:40:21     INFO -      Found by: given as instruction pointer in context
20:40:21     INFO -   1  libxul.so!js::ScriptFrameIter::computeThis() const [Interpreter-inl.h:c98c47341be6 : 53 + 0x15]
20:40:21     INFO -      eip = 0xb5e0c03a   esp = 0xbfb212b0   ebp = 0xbfb212e8   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0xaea5e781   edi = 0x00000001
20:40:21     INFO -      Found by: call frame info
20:40:21     INFO -   2  libxul.so!DebuggerFrame_getThis [Debugger.cpp:c98c47341be6 : 3822 + 0x7]
20:40:21     INFO -      eip = 0xb5daf0a5   esp = 0xbfb212f0   ebp = 0xbfb21488   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0xbfb21338   edi = 0xb724aa60
20:40:21     INFO -      Found by: call frame info
20:40:21     INFO -   3  libxul.so!js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [jscntxtinlines.h:c98c47341be6 : 225 + 0x12]
20:40:21     INFO -      eip = 0xb5dbfb5c   esp = 0xbfb21490   ebp = 0xbfb216b8   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0x8ac0b5b0   edi = 0xbfb2170c
20:40:21     INFO -      Found by: call frame info
20:40:21     INFO -   4  libxul.so!js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) [Interpreter.cpp:c98c47341be6 : 527 + 0x1e]
20:40:21     INFO -      eip = 0xb5dcfe7b   esp = 0xbfb216c0   ebp = 0xbfb21768   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0xb724aa60   edi = 0x00000000
20:40:21     INFO -      Found by: call frame info
20:40:21     INFO -   5  libxul.so!js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) [Interpreter.cpp:c98c47341be6 : 598 + 0x27]
20:40:21     INFO -      eip = 0xb5dd0023   esp = 0xbfb21770   ebp = 0xbfb217b8   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0xb724aa60   edi = 0xbfb21ae0
20:40:21     INFO -      Found by: call frame info
20:40:21     INFO -   6  libxul.so!js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>) [Shape-inl.h:c98c47341be6 : 261 + 0x28]
20:40:21     INFO -      eip = 0xb5f07418   esp = 0xbfb217c0   ebp = 0xbfb21828   ebx = 0xb6e1e8bc
20:40:21     INFO -      esi = 0xb724aa60   edi = 0xbfb21ae0
20:40:21     INFO -      Found by: call frame info
20:40:21     INFO -   7  libxul.so!js::GetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<int>, unsigned int, JS::MutableHandle<JS::Value>) [jsobj.cpp:c98c47341be6 : 4026 + 0x29]
Summary: Intermittent TEST-UNEXPECTED-FAIL | /devtools/debugger/test/browser_dbg_blackboxing-02.js | Exited with code 11 during test run |application crashed [@ js::BoxNonStrictThis(JSContext*, JS::MutableHandle<JS::Value>, bool*)] → Permaorange fx-team devtools/debugger/test/browser_dbg_blackboxing-02.js | application crashed [@ js::BoxNonStrictThis(JSContext*, JS::MutableHandle<JS::Value>, bool*)]
this might been cause by:

-> https://hg.mozilla.org/integration/fx-team/rev/a58e072e2ae7
-> it then relanded again as 9c0275e6303e
->https://hg.mozilla.org/integration/fx-team/log/a4e8ace8a4a9/browser/devtools/debugger/test/browser_dbg_blackboxing-02.js

Nick, could you take a look, since this is also now perma-organge on fx-team i close that tree
Flags: needinfo?(nfitzgerald)
This is probably a SpiderMonkey bug that Nick's otherwise benign front-end change seems to trigger. CCing Jim and Eddy who may be able to help figure this one out.
Potentially related to bug 880538?
(In reply to Dave Camp (:dcamp) from comment #6)
Highly doubtful unless you've got any "use asm" in this test suite.
Also bug 894948 was the last one to touch the files containing the top stack frames in the crash dump.
I don't see how bug 894948 could cause this: it didn't land anything, effectively.
The original push which landed that test was all green: https://tbpl.mozilla.org/?tree=Fx-Team&rev=9c0275e6303e

So it has to be something that landed after, right?
Flags: needinfo?(nfitzgerald)
How come the crash only happens in opt builds, while debug builds run fine? What do we do differently there?
cx->global() dereferences cx->compartment() so this would have been crashing already.
I can reproduce the crash in my old Mac, if anyone wants me to test anything.
Oh, so it could have been caused by bug 894948, after all: the backout apparently wasn't pushed with the original patch. RyanVM pushed it just now.
(In reply to Till Schneidereit [:till] from comment #17)
> Oh, so it could have been caused by bug 894948, after all: the backout
> apparently wasn't pushed with the original patch. RyanVM pushed it just now.

(In reply to TinderboxPushlog Robot from comment #18)
> RyanVM
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25498688&tree=Fx-Team
> Rev5 MacOSX Mountain Lion 10.8 fx-team opt test mochitest-browser-chrome on
> 2013-07-19 11:50:10
> revision: 20848adc9980
> slave: talos-mtnlion-r5-022
> 
> 12:01:32  WARNING -  TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_blackboxing-02.js | Exited with code 1 during test run
> 12:02:02  WARNING -  PROCESS-CRASH |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_blackboxing-02.js | application crashed [@
> js::BoxNonStrictThis(JSContext*, JS::MutableHandle<JS::Value>, bool*)]
> 12:02:03    ERROR - Return code: 256

Guess not.
backed out my patch until we can figure out who the offender is: https://hg.mozilla.org/integration/fx-team/rev/24b00dfb20de
Bisecting pointed to bug 887334 as the culprit. I'm now bisecting through the many changesets in that bug to figure out which one it is.
The first bad revision is:

https://hg.mozilla.org/integration/fx-team/rev/5427f4d376c4

Bug 887334 - Stop setting the compartment to defaultCompartmentObject_->compartment(). r=luke
bholley: I landed a patch (bug 877686) on fx-team that introduced a new test, the test results for the whole commit were green, then we merged m-c into fx-team, and this new test crashed the browser every time. Note that my patch doesn't touch any platform code, and is only some JS for the debugger frontend.

After bisecting, your commit for bug 887334 was revealed to be the changeset that introduced this crashing behavior. Can you take a look at this? I can't land my patch until this is fixed.

Thanks!
Flags: needinfo?(bobbyholley+bmo)
This only reproduces on opt builds, so I've been fighting a bit with garbled stacks. But the basic situation seems to be that we call DebuggerFrame_getThis, which enters a compartment here:

http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#3822

But that ends up invoking ScriptFrameIter::computeThis, which pull what seems to be a different cx out of data_, and passes it to ComputeThis:

http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Stack.cpp#1107

That cx has a null compartment_, presumably meaning that nobody ever entered a compartment (at least since the last call to saveFrameChain). In the old days, we'd have been in the default compartment for the cx. But now we crash when we try to call ->global(), since that assumes we're in a compartment.

I don't know a ton about this ScriptFrameIter stuff, so it's hard for me to tell what's going wrong, and gdb-ing this stuff isn't productive given the opt-ness. Maybe luke can tell me more, or suggest somebody who knows this stuff better to dig in?
Flags: needinfo?(bobbyholley+bmo) → needinfo?(luke)
Given bug 868437, jandem might be able to help.
Flags: needinfo?(jdemooij)
Attached patch maybe the fix? (obsolete) — Splinter Review
Ah hah, iiuc, this is might be a pre-existing cross-compartment bug:
The ScriptFrameIter walks down a callstack where each frame can be in a different compartment, therefore no assumption can be made about the compartment of a frame (as canonically determined by its fp->scopeChain->compartment) and cx->compartment.  Thus, it is a potential cross-compartment violation for ScriptFrameIter::computeThis to call ComputeThis without having AutoCompartment'd fp->scopeChain (in particular this could allocate objects (via js_PrimitiveToObject) in the wrong compartment and then store these to fp->thisv!  bholley's patch just made the disconnect between cx->compartment and fp->scopeChain->compartment more evident :)

Nick: could you apply this patch and test whether it fixes the problem you're seeing?  In the meantime I'll try to cook up a shell testcase...
Attachment #779417 - Flags: review?(jdemooij)
Attachment #779417 - Flags: feedback?(nfitzgerald)
Flags: needinfo?(luke)
Attachment #779417 - Attachment is patch: true
Severity: normal → critical
Attached patch fix and testSplinter Review
Getting shell testcases is always informative; I was able to get a compartment violation assert to trigger, but it requires not just cross-compartment calls but also multiple JSContexts to be used.  It turns out, callers of computeThis are already entering the right compartment (they're about to touch the thisValue after all and we have compartment asserts a'plenty); the issue is that we use the JSContext of the StackFrame which may be different than the JSContext of the debugger.  Thus, this patch just adds a JSContext *cx parameter (which is good form anyhow, for fallible methods).

So this patch definitely fixes *a* bug; waiting for Nick to hear whether the patch fixes the above crash.
Attachment #779417 - Attachment is obsolete: true
Attachment #779417 - Flags: review?(jdemooij)
Attachment #779417 - Flags: feedback?(nfitzgerald)
Attachment #779472 - Flags: review?(jdemooij)
Attachment #779472 - Flags: feedback?(nfitzgerald)
Flags: needinfo?(jdemooij)
Applying that patch fixed it for me locally, but here is a try push to be sure: https://tbpl.mozilla.org/?tree=Try&rev=79bce0f13e3d
Ok with the new patch, it isn't crashing locally, and here is a try push: https://tbpl.mozilla.org/?tree=Try&rev=cf1b337067ab
I think that try push is good; I'm assuming f+ Nick?
Comment on attachment 779472 [details] [diff] [review]
fix and test

Yeah looks good to me.
Attachment #779472 - Flags: feedback?(nfitzgerald) → feedback+
Comment on attachment 779472 [details] [diff] [review]
fix and test

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

Good catch.
Attachment #779472 - Flags: review?(jdemooij) → review+
Oops, it marked me as the author... big enough deal to backout and re-commit?
(In reply to Nick Fitzgerald [:fitzgen] from comment #41)
> Oops, it marked me as the author... big enough deal to backout and re-commit?

If someone hasn't set the author in their global hgrc, I see it as finders keepers hehe ;-)
That's strange, I do have username set in hgrc; no big deal though.  Just to make sure I understand, fx-team will eventually merge to m-c?
Probably my fault somewhere along the qimport, qref -m "...", qfinish sequence.

Yes, fx-team will merge into m-c.
https://hg.mozilla.org/mozilla-central/rev/978b2693a7a4
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla25
(In reply to Bobby Holley (:bholley) from comment #46)
> https://hg.mozilla.org/releases/mozilla-aurora/
> pushloghtml?changeset=84b828b63115

Backed out from Aurora for possibly causing xpcshell crashes along with the other changes from bholley's push in https://hg.mozilla.org/releases/mozilla-aurora/rev/659b0d61fbc6
You need to log in before you can comment on or make changes to this bug.