488802 - Crash at [@js_Interpret] on YOMIURI ONLINE

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

bug 488015 was fixed, however, I can still reproduce similar crash at YOMIURI ONLINE which is major newspaper site in Japan.

Load the URL: http://www.yomiuri.co.jp/

http://crash-stats.mozilla.com/report/index/a908ff6d-319e-4458-b543-8e60f2090416
> 0  	js3250.dll  	js_Interpret  	 js/src/jsinterp.cpp:5737
> 1 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1388
> 2 	js3250.dll 	js_InternalInvoke 	js/src/jsinterp.cpp:1441
> 3 	js3250.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5196
> 4 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2007
> 5 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:7736
> 6 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:8070
> 7 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:427
> 8 	nspr4.dll 	_PR_MD_UNLOCK 	nsprpub/pr/src/md/windows/w95cv.c:344
> 9 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:519
> 10 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
> 11 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
> 12 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:192
> 13 	nspr4.dll 	PR_GetEnv 	
> 14 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:110
> 15 	firefox.exe 	firefox.exe@0x21a7 	
> 16 	kernel32.dll 	kernel32.dll@0x8e4a4 	
> 17 	ntdll.dll 	ntdll.dll@0x7cfec 	
> 18 	ntdll.dll 	ntdll.dll@0x7d1fe 

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090416 Minefield/3.6a1pre (.NET CLR 3.5.30729)

Comment 1

[Brendan Eich [:brendan]]

It's a different bug, same symptom. Easy to fix: the analysis tests for use of arguments in the flat closure optimization path, but not in the display-based null closure path. At the test URL, in a big prototype object initialiser we have

        scroll: function(){
                if (this.moving && !this.refresh) return;
                var items = this.ul_o;
                items.scrollLeft =0;
                var differ = this.itemsW-this.frameW-this.buttonsW;
                var obj = this;
                if (this.itemsW>this.frameW) {
                        this.changeButton([this.btn_l,this.btn_r],'on');
                        this.moving = true;
                        itr();
                } else {
                        this.changeButton([this.btn_l,this.btn_r],'off');
                }
                function itr(){
                        var ofst =  + (obj.mRange) ? obj.mRange : obj.offset;
                        if (ofst || obj.refresh) {
                                items.scrollLeft += ofst;
                                if (items.scrollLeft ==0) {
                                        obj.changeButton([obj.btn_l],'off');
                                        obj.offset = obj.speed;
                                } else
                                if (items.scrollLeft>= differ) {
                                        obj.changeButton([obj.btn_r],'off');
                                        obj.offset = -obj.speed;
                                } else {
                                        obj.changeButton([obj.btn_l,obj.btn_r],'on');
                                }
                                if (obj.refresh) obj.refresh=false;
                        }
                        setTimeout(arguments.callee,100);
                };
        },

The itr function does not escape and it uses the items, differ, and obj upvars only from its lexical parent scope, where the only call by its name is made. But of course it ends with setTimeout(arguments.callee,100). D'oh!

This should block. Patch in a minute.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Philosophy is still to note facts needed by the analysis as early as possible. We may clear PND_FUNARG but only if TCF_USES_ARGUMENTS is not set. So we could test only TCF_USES_ARGUMENTS in the analysis, but that would require two tests then: isFunArg() && <uses-arguments>.

Instead, provided we have tc->funbox (i.e., we are not compiling a function body, e.g. for XBL or an event handler), we induce PND_FUNARG promptly.

There's a bit of sanity-checking here to avoid setting TCF_FUN_* for a top-level context (not function code), in Variables.

/be

Comment 3

[Brendan Eich [:brendan]]

I should have written "So, for this bug fix, we could test TCF_USES_ARGUMENTS in addition to isFunArg()..." -- obviously one test is better than two, which was my point. Anyway...

Thanks very much for the bug report, Masayuki-san!

/be

Comment 4

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/10429fdd5346

/be

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/10429fdd5346

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you Brendan, I verified this with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090418 Minefield/3.6a1pre (.NET CLR 3.5.30729)

Comment 8

[Alice0775 White]

In Shiretok, it still crashes. 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090419 Shiretoko/3.5b4pre ID:20090419043945

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #8)
> In Shiretok, it still crashes. 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090419
> Shiretoko/3.5b4pre ID:20090419043945

Hey, please check the pushlog. The patch wasn't landed to 1.9.1 branch.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/93cfb45c872c

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090423 Shiretoko/3.5b4pre ID:20090423040926