884194 - crash in Interpret

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Scoobidiver (away)]

It first showed up in 24.0a1/20130615, likely a signature morphing of bug 682573, and started spiking in 24.0a1/20130617 (#4 top crasher in that build so higher than bug 682573 in other channels). The regression ranges are:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b197bed90a98&tochange=3d16d59c9317
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=36da3cb92193&tochange=834c8941ae24 (spike)

Stack traces are various:
Frame 	Module 	Signature 	Source
0 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp:2465
1 	mozjs.dll 	js::RunScript 	js/src/vm/Interpreter.cpp:328
2 	mozjs.dll 	js::Invoke 	js/src/vm/Interpreter.cpp:400
3 	mozjs.dll 	js_fun_call 	js/src/jsfun.cpp:828

Frame 	Module 	Signature 	Source
0 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp:2465
1 	mozjs.dll 	js::RunScript 	js/src/vm/Interpreter.cpp:328
2 	mozjs.dll 	js::Invoke 	js/src/vm/Interpreter.cpp:437
3 	mozjs.dll 	js::ion::DoCallFallback 	js/src/ion/BaselineIC.cpp:6997

Frame 	Module 	Signature 	Source
0 	libxul.so 	Interpret 	js/src/vm/Interpreter.cpp:2465
1 	libxul.so 	void js::assertSameCompartment<JS::Handle<JSObject*> > 	js/src/jscntxtinlines.h:206
2 	libxul.so 	JS_BasicObjectToString 	js/src/builtin/Object.cpp:301
3 	libxul.so 	JS_EnumerateStub 	js/src/jsapi.cpp:3235
4 	libxul.so 	obj_toString 	js/src/builtin/Object.cpp:328

Frame 	Module 	Signature 	Source
0 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp:1852
1 	mozjs.dll 	js::RunScript 	js/src/vm/Interpreter.cpp:328
2 	mozjs.dll 	js::ExecuteKernel 	js/src/vm/Interpreter.cpp:533
3 	mozjs.dll 	js::Execute 	js/src/vm/Interpreter.cpp:573
4 	mozjs.dll 	JS::Evaluate 	js/src/jsapi.cpp:5690
5 	xul.dll 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1278
6 	xul.dll 	nsScriptLoader::EvaluateScript 	content/base/src/nsScriptLoader.cpp:854
7 	xul.dll 	nsScriptLoader::ProcessRequest 	content/base/src/nsScriptLoader.cpp:742
...

Frame 	Module 	Signature 	Source
0 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp:2465
1 	mozjs.dll 	js::RunScript 	js/src/vm/Interpreter.cpp:348
2 	mozjs.dll 	js::Invoke 	js/src/vm/Interpreter.cpp:437
3 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5886
4 	xul.dll 	mozilla::dom::Function::Call 	obj-firefox/dom/bindings/FunctionBinding.cpp:39
5 	xul.dll 	mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > 	obj-firefox/dist/include/mozilla/dom/FunctionBinding.h:52
6 	xul.dll 	nsGlobalWindow::RunTimeoutHandler 	dom/base/nsGlobalWindow.cpp:10209
...

Frame 	Module 	Signature 	Source
0 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp:2465
1 	xul.dll 	nsDisplayBackgroundImage::AppendBackgroundItemsToTop 	layout/base/nsDisplayList.cpp:1643
2 	xul.dll 	nsBoxFrame::BuildDisplayList 	layout/xul/base/src/nsBoxFrame.cpp:1346
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=Interpret

Comment 1

[Alice0775 White]

bp-9a8a2540-dd26-4388-bd31-2af262130618

http://hg.mozilla.org/mozilla-central/rev/4e5983de6e3b
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130618 Firefox/24.0 ID:20130618031335

I can reproduce the crash in Linux with STR of Bug 884156

Comment 2

[Alice0775 White]

bp-6c8f9d1c-2792-431a-8e0e-48a032130619

Steps To Reproduce:
1. Open Web Console
2. Open http://blog.feedly.com/2013/03/14/google-reader/

Actual Results:
Browser crashes

Comment 3

[Alice0775 White]

Regression window with STR  of comment#2 is as follows.

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8a22078d93b2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130615 Firefox/24.0 ID:20130616061322
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/91f620586eb8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130615 Firefox/24.0 ID:20130616092524
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8a22078d93b2&tochange=91f620586eb8

Regressed by: 
91f620586eb8	Brian Hackett — Bug 883630 - Watch for lazy functions when iterating inline Ion frames, clean up methods for accessing function scripts.

Comment 4

[Loic]

Annoying bug, Firefox crashes on various websites when the web console is open.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

Sorry for the delay.  The getExistingScript() function added in the blamed cset does not update fun->isHeavyweight() and fun->nargs as it should.  If such functions, or their clones, show up on scope chains then scope objects do not get pushed on the scope chain when they should and havoc ensues.  The attached patch fixes this and renames getExistingScript per an earlier review request.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 765442 [details] [diff] [review]
patch

r+ for this quick fix.

To make this all less error prone, could we change this so that JSFunction fields don't have to be updated lazily?  In particular:
 - it seems like we could move the heavyweight bit to JSScript
 - fun->nargs belongs on JSFunction but could it be set eagerly/correctly when initially creating the lazy JSFunction?

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #6)
> To make this all less error prone, could we change this so that JSFunction
> fields don't have to be updated lazily?  In particular:
>  - it seems like we could move the heavyweight bit to JSScript
>  - fun->nargs belongs on JSFunction but could it be set eagerly/correctly
> when initially creating the lazy JSFunction?

Yeah, I thought about this but the problem is with nargs.  We can't set the bit eagerly for the JSFunction on delazification because at that point there may be many clones pointing to the same LazyScript, all of which would need to be updated.  So handling nargs correctly would require that they be tracked in the JSScript for interpreted functions, which could bloat JSScript without shrinking JSFunction.  Now, JSScript has a PADDING16 field but it'd be nice to leave that as padding and find other stuff to kill or move (version, staticLevel, length, natoms ...) to shrink the structure further.

Comment 8

[Luke Wagner [:luke]]

Actually, I'm confused now: looking at functionArgsAndBody, it seems like we will correctly set fun->nargs in functionArgsAndBodyGeneric.  How do a lazy function's nargs get to be wrong?

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #8)
> Actually, I'm confused now: looking at functionArgsAndBody, it seems like we
> will correctly set fun->nargs in functionArgsAndBodyGeneric.  How do a lazy
> function's nargs get to be wrong?

Oops, yes, you're right, ParseContext<SyntaxParseHandler>::define updates args_ so that fun->nargs should always be right.  I'll write a patch to kill JSFunction::HEAVYWEIGHT.

Comment 10

[Luke Wagner [:luke]]

Great, thanks!

Comment 11

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/208bb5954938

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/208bb5954938

Comment 14

[Mihai Morar, (:MihaiMorar)]

Not able to reproduce the crash following STR from Bug 883562 on FF 24b6. Not marking as Verified because there are still lots of crashes on Soccoro on FF 24b5 which was released after Ed's pushlog from Comment 12

https://crash-stats.mozilla.com/report/list?signature=Interpret&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-08-27+14%3A00%3A00&range_value=1

Comment 15

[Manuela Muntean [Away]]

I wasn't able to reproduce this crash on Win 7 64bit, using STR from comment 2 and bug Bug 884156, so I tried on Ubuntu 13.04 32bit, and I managed to reproduce the crash using STR from Bug 884156.

This crash doesn't reproduce with Firefox 24 beta 9 (build ID: 20130905180733) on Ubuntu, with both STR.

Also, there aren't many crashes since 24.0b5 in Socorro, regarding last month:

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&reason_type=contains&date=2013-08-27&range_value=28&range_unit=days&hang_type=any&process_type=any&signature=Interpret

As a conclusion, marking this verified fixed.

Comment 16

[Michael Kraft [:morac]]

I'm not sure this is fixed as I just got this crash after upgrading to Firefox 24 on Win 7 64bit.  I've never had this crash prior to that.

d9bab320-d60e-4da4-8885-b24782130918

Comment 17

[Loic]

Link to your CR:
https://crash-stats.mozilla.com/report/index/d9bab320-d60e-4da4-8885-b24782130918

Are you able to crash by following the STR in comment #2?

Comment 18

[Michael Kraft [:morac]]

No,

I've started getting a variety of different crash signatures (usually including js::GCSlice(JSRuntime *,js::JSGCInvocationKind,JS::gcreason::Reason,__int64)) so it might be something else.

Comment 19

[Loic]

Ok, open a new bug report if you're able to find some STR to reproduce these new crashes.