Crash [@ js::mjit::ic::GetProp ]

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Scoobidiver (away), Assigned: billm)

Tracking

(Blocks: 1 bug, {crash})

Trunk
mozilla8
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6 affected, firefox7 affected, firefox8 affected, status1.9.2 unaffected)

Details

(Whiteboard: [js-triage-done], crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
It is #26 top crasher in Fennec 4.0b4 over the last week.

There are two kinds of stack traces:
Frame 	Module 	Signature [Expand] 	Source
0 	libxul.so 	js::mjit::ic::GetProp 	js/src/jsobj.h:1185
1 	libxul.so 	libxul.so@0xb1d10a 	
2 	libxul.so 	js::mjit::ic::GetProp 	js/src/methodjit/PolyIC.cpp:1635
3 	libxul.so 	js::mjit::JaegerShot 	js/src/jscntxt.h:2890
4 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:654
5 	libxul.so 	js::ExternalInvoke 	js/src/jsinterp.cpp:858
6 	libxul.so 	JS_CallFunctionValue 	js/src/jsinterp.h:961
7 	libxul.so 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2007
8 	libxul.so 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:230
9 	libxul.so 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1114
10 	libxul.so 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:1209
11 	libxul.so 	nsEventTargetChainItem::HandleEvent 	content/events/src/nsEventListenerManager.h:146
12 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:343
13 	libxul.so 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:630
...

Frame 	Module 	Signature [Expand] 	Source
0 	libxul.so 	js::mjit::ic::GetProp 	js/src/jsobj.h:1185
1 	libxul.so 	libxul.so@0xb1d10a 	
2 	libxul.so 	js::mjit::ic::GetProp 	js/src/methodjit/PolyIC.cpp:1635
3 	libxul.so 	js::mjit::JaegerShot 	js/src/jscntxt.h:2890
4 	libxul.so 	js::Execute 	js/src/jsinterp.cpp:654
5 	libxul.so 	JS_EvaluateUCScriptForPrincipals 	js/src/jsapi.cpp:4933
6 	libxul.so 	JS_EvaluateUCScriptForPrincipalsVersion 	js/src/jsapi.cpp:151
7 	libxul.so 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1552
8 	libxul.so 	nsScriptLoader::EvaluateScript 	nsCOMPtr.h:655
9 	libxul.so 	nsScriptLoader::ProcessRequest 	nsCOMPtr.h:800
10 	libxul.so 	nsScriptLoader::ProcessScriptElement 	content/base/src/nsScriptLoader.cpp:646
11 	libxul.so 	nsScriptElement::MaybeProcessScript 	content/base/src/nsScriptElement.cpp:185
12 	libxul.so 	nsHTMLScriptElement::MaybeProcessScript 	content/html/content/src/nsHTMLScriptElement.cpp:584
13 	libxul.so 	nsHTMLScriptElement::DoneAddingChildren 	content/html/content/src/nsHTMLScriptElement.cpp:511
14 	libxul.so 	nsHtml5TreeOpExecutor::RunScript 	parser/html/nsHtml5TreeOpExecutor.cpp:734
15 	libxul.so 	nsHtml5TreeOpExecutor::RunFlushLoop 	parser/html/nsHtml5TreeOpExecutor.cpp:528
16 	libxul.so 	nsHtml5ExecutorReflusher::Run 	parser/html/nsHtml5TreeOpExecutor.cpp:92
17 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633

It is probably a dupe of bug 615993.

More reports at:
https://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=js%3A%3Amjit%3A%3Aic%3A%3AGetProp

Comment 1

7 years ago
another one: https://crash-stats.mozilla.com/report/index/bp-edfe7b25-9669-42e8-9ac6-390892110506
Crash Signature: [@ js::mjit::ic::GetProp ]

Comment 2

6 years ago
1. http://www.m1982.com/player/33315.html?33315-0-3
2. Crash (may need to wait a while)

nightly Nightly/8 winxp
bp-5ce59278-8826-430f-a45b-cc0fc2110723
[@ js::mjit::ic::GetProp ]

nightly Aurora/7 winxp No Crash

nightly Beta/6 winxp 
bp-196f0d63-86dc-4a49-a81f-bfc372110723
[@ JSObject::nativeSearch(int, bool) ] 
This matches the original socorro signature used in the crash automation.

Automation reproduced crashes in Debug Beta/6, Aurora/7 Win7/Linux with stack JSObject::getProperty(JSContext*, JSObject*, jsid, js::Value*) JSObject::getProperty(JSContext*, jsid, js::Value*) js::mjit::ic::GetProp(js::VMFrame&, js::mjit::ic::PICInfo*) js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, js::Value*)

locally reproduced with debug aurora Mac OS X 10.5
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xdadadb3a
0x06ab81bc in JSObject::getProperty (this=0x1a534e40, cx=0x24c0c9d0, receiver=0x1a534e40, id={asBits = 431187008}, vp=0xbfff5e28) at jsobj.h:1149
1149	        js::PropertyIdOp op = getOps()->getProperty;
(gdb) bt
#0  0x06ab81bc in JSObject::getProperty (this=0x1a534e40, cx=0x24c0c9d0, receiver=0x1a534e40, id={asBits = 431187008}, vp=0xbfff5e28) at jsobj.h:1149
#1  0x06be518d in JSObject::getProperty (this=0x1a534e40, cx=0x24c0c9d0, id={asBits = 431187008}, vp=0xbfff5e28) at jsobj.h:1154
#2  0x06be2e5b in js::mjit::ic::GetProp (f=@0xbfff5e70, pic=0x2a0185ac) at /work/mozilla/builds/aurora/mozilla/js/src/methodjit/PolyIC.cpp:1786
#3  0x1fd516c9 in ?? ()
#4  0x06b8446a in js::mjit::EnterMethodJIT (cx=0x24c0c9d0, fp=0x1a0ac3a0, code=0x1e2b03fc, stackLimit=0x1a4ac000) at /work/mozilla/builds/aurora/mozilla/js/src/methodjit/MethodJIT.cpp:686
#
Blocks: 532972
Crash Signature: [@ js::mjit::ic::GetProp ] → [@ js::mjit::ic::GetProp ][@ JSObject::nativeSearch(int, bool) ]
status-firefox6: --- → affected
status-firefox7: --- → affected
status-firefox8: --- → affected
OS: Android → All
Hardware: ARM → All
(Assignee)

Comment 3

6 years ago
I wasn't able to reproduce this, but I don't have flash.
Whiteboard: js-triage-needed
(Assignee)

Comment 4

6 years ago
I think I tracked this down. The problem is in jsop_callprop_str in the methodjit. Here's the relevant code:

    /*
     * Bake in String.prototype. This is safe because of compileAndGo.
     * We must pass an explicit scope chain only because JSD calls into
     * here via the recompiler with a dummy context, and we need to use
     * the global object for the script we are now compiling.
     */
    JSObject *obj;
    if (!js_GetClassPrototype(cx, &fp->scopeChain(), JSProto_String, &obj))
        return false;

    /* Force into a register because getprop won't expect a constant. */
    RegisterID reg = frame.allocReg();

    masm.move(ImmPtr(obj), reg);
    frame.pushTypedPayload(JSVAL_TYPE_OBJECT, reg);

    /* Get the property. */
    if (!jsop_getprop(atom))
        return false;

I think this is what's happening:
  1. compile the script
  2. clear the scope, which overwrites JSProto_String with undefined in the global
  3. GC, which collects the former JS_ProtoScope object if it's otherwise unrooted
  4. run the code, try to access that object, and crash

If I run the code in the interpreter, I get an error about trying to run a compile-and-go script with a cleared scope. I don't seem to get this error when running with the methodjit. Waldo thinks we should be able to make the methodjit generate the same error. However, I'm having trouble figuring out how we actually enter this script. The call stack is garbage.

Ccing Jason and David. Maybe you guys know more about why we don't generate this error in mjitted code?

I can see three possible fixes:
  1. generate the error in all cases
  2. don't bake in the address
  3. if we do bake it in, root it properly
Group: core-security
Whiteboard: js-triage-needed → js-triage-done
(In reply to comment #4)
> I think this is what's happening:
>   1. compile the script
>   2. clear the scope, which overwrites JSProto_String with undefined in the
> global

Does "clear the scope" mean calling JS_ClearScope? JS_ClearScope on live objects is really bad, and bug 637099 is supposed to remove it. I thought that was almost done, but now maybe not.

>   3. GC, which collects the former JS_ProtoScope object if it's otherwise
> unrooted
>   4. run the code, try to access that object, and crash
> 
> If I run the code in the interpreter, I get an error about trying to run a
> compile-and-go script with a cleared scope. I don't seem to get this error
> when running with the methodjit. Waldo thinks we should be able to make the
> methodjit generate the same error. However, I'm having trouble figuring out
> how we actually enter this script. The call stack is garbage.
> 
> Ccing Jason and David. Maybe you guys know more about why we don't generate
> this error in mjitted code?
> 
> I can see three possible fixes:
>   1. generate the error in all cases

I think that's hard, because scripts can call each other, so every function call would have to be instrumented.

>   2. don't bake in the address

Not good for perf.

>   3. if we do bake it in, root it properly

Seems like the way to go.

Comment 6

6 years ago
I've seen the attempt to run compile-and-go script on a cleared scope message on other urls as well even though they haven't crashed or asserted. 15 so far today.
(In reply to comment #5)
> >   3. if we do bake it in, root it properly
> 
> Seems like the way to go.

Agreed.
Someone please save the whole web pages and upload a test case. We've had such test cases disappear from the web before.

(In reply to comment #6)
> I've seen the attempt to run compile-and-go script on a cleared scope
> message on other urls as well even though they haven't crashed or asserted.
> 15 so far today.

If you get the error, it means that path is safe; we stopped you from entering the interpreter (or methodjit code). Apparently there is a path where we don't check.

Comment 9

6 years ago
(In reply to comment #8)
> Someone please save the whole web pages and upload a test case. We've had
> such test cases disappear from the web before.

I've tried and failed to get a reproducible test case.
(Assignee)

Comment 10

6 years ago
Peter, do you know whether it's possible for JS_ClearScope to happen while JS code is running? If it's safe to do so, Jason and I would like to add a !JS_IsRunning(cx) assertion to JS_ClearScope.
(Assignee)

Comment 11

6 years ago
Created attachment 549431 [details] [diff] [review]
patch

I think this should fix the immediate problem. I'll add the assertion if it's warranted.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #549431 - Flags: review?(dmandelin)
Comment on attachment 549431 [details] [diff] [review]
patch

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

Looks great.

::: js/src/methodjit/MethodJIT.cpp
@@ +842,5 @@
>  
> +JSObject **
> +JITScript::rootedObjects() const
> +{
> +    return (JSObject **)((char *)callSites() + sizeof(js::mjit::CallSite) * nCallSites);

Would 

  (JSObject **)(callSites() + nCallSites)

work too, or does that miss due to some detail of pointer conversions?
Attachment #549431 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 13

6 years ago
(In reply to comment #12)
> Would 
> 
>   (JSObject **)(callSites() + nCallSites)
> 
> work too, or does that miss due to some detail of pointer conversions?

Yeah, I think that should work. I'm a bit worried that GCC will barf out a ton of strong aliasing warnings, but I'll check. In fact, this would probably be even cleaner:
  (JSObject **)&callSites()[nCallSites]
Created attachment 549477 [details]
Saved web test case
Blocks: 595635
(Assignee)

Updated

6 years ago
Whiteboard: js-triage-done → [js-triage-done][inbound]
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0f4acd33ac43
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [js-triage-done][inbound] → [js-triage-done]
Target Milestone: --- → mozilla8
Daniel let me know that this is "a bug in code that was basically rewritten from scratch for Firefox 4, so Firefox 3.6 shouldn't be affected". Marking as status1.9.2:unaffected.
status1.9.2: --- → unaffected
Sorry, wrong comment, right status...

Bill let me know that this "does not affect 3.6 because it is a methodjit bug".
Group: core-security
You need to log in before you can comment on or make changes to this bug.