41948 – REGRESSION(r60392): Registerfile can be unwound too far following an exception

RESOLVED FIXED 41948

REGRESSION(r60392): Registerfile can be unwound too far following an exception

https://bugs.webkit.org/show_bug.cgi?id=41948

Summary REGRESSION(r60392): Registerfile can be unwound too far following an exception

Peter Speck

Reported 2010-07-09 03:26:43 PDT

JSC crashes when visiting the page: http://jp.dk/udland/europa/article2121366.ece Works in Safari 5, r58209 + r59902 Crashes r60462 + r61502 + r60820 + r61979 + r62096 + r62241 + r62632 Process: Safari [95225] Path: /Applications/Safari.app/Contents/MacOS/Safari Identifier: org.webkit.nightly.WebKit Version: r62632 (62632) Code Type: X86-64 (Native) Parent Process: launchd [169] Date/Time: 2010-07-09 12:02:13.908 +0200 OS Version: Mac OS X 10.6.4 (10F569) Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: 0x000000000000000d, 0x0000000000000000 Crashed Thread: 0 Dispatch queue: com.apple.main-thread Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000100840977 JSC::JSObject::defaultValue(JSC::ExecState*, JSC::PreferredPrimitiveType) const + 4039 1 com.apple.JavaScriptCore 0x000000010076138d JSC::JSObject::toPrimitive(JSC::ExecState*, JSC::PreferredPrimitiveType) const + 13 2 com.apple.JavaScriptCore 0x000000010083ea09 JSC::JSObject::toString(JSC::ExecState*) const + 57 3 com.apple.JavaScriptCore 0x00000001008c4bfe JSC::stringProtoFuncSubstring(JSC::ExecState*) + 526 4 ??? 0x00004c94dd2001aa 0 + 84202248733098 5 com.apple.JavaScriptCore 0x00000001007cfab0 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*, JSC::JSValue*) + 528 6 ??? 0x0000000117e5ea00 0 + 4695910912

Attachments
reduction (112 bytes, text/html) 2010-07-14 13:28 PDT, Geoffrey Garen	no flags	Details
potential solution (669 bytes, patch) 2010-07-20 14:53 PDT, Geoffrey Garen	no flags	Details Formatted Diff Diff
The patch (3.95 KB, patch) 2010-09-28 14:49 PDT, Gavin Barraclough	barraclough: review- ggaren: commit-queue-	Details Formatted Diff Diff
patch (4.81 KB, patch) 2010-10-20 13:26 PDT, Geoffrey Garen	no flags	Details Formatted Diff Diff
patch (7.46 KB, patch) 2010-10-26 18:22 PDT, Geoffrey Garen	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Peter Speck

Comment 1 2010-07-09 03:27:13 PDT

Works in r60376 + r60332 + r59902 + r58209 + r57720 + Safari 5. Crashes r60462 + r60820 + r61502 + r61979 + r62096 + r62241 + r62632 There is no nightly between r60376 and r60462. JSC patches in this range: r60392, r60378, r60390, r60401

Mark Rowe (bdash)

Comment 2 2010-07-09 05:01:00 PDT

This was caused by <http://trac.webkit.org/changeset/60392>.

Mark Rowe (bdash)

Comment 3 2010-07-09 05:01:27 PDT

<rdar://problem/8174724>

Geoffrey Garen

Comment 4 2010-07-09 10:58:18 PDT

Mark, can you reproduce this crash? I couldn't.

Peter Speck

Comment 5 2010-07-09 12:24:48 PDT

I can reduce the html page to the following (saved as a local file), and still have it crash every time. Opening the iframe by itself doesn't make it crash. <html> <head> </head> <body> <iframe src='http://front.xstream.dk/jptv/player/player_small/?id=23550' width="480" height="402" scrolling="no" frameborder="no" border="0" frame spacing="0" marginheight="0" marginwidth="0" vspace="0" hspace="0" ></iframe> </body> </html> If I save the iframe html to a local file (same dir) and uses a relative url in <iframe>, then it doesn't crash. Element pane in Inspector is always enabled.

Mark Rowe (bdash)

Comment 6 2010-07-09 12:56:14 PDT

Geoff, yes, I can reproduce this 100% of the time.

Peter Speck

Comment 7 2010-07-09 13:12:59 PDT

Trimming the iframe scripts using GlimmerBlocker (http proxy which can modify files on-the-fly), I can reduce the iframe to be just the script http://front.xstream.dk/jptv/resources/scripts/flash_streaming.js and still have it crash. I can reduce the iframe script, so it only consists of the following: var href=new String(document.location.href);var f=0;var fv='-'; var ref = 'hello' eval('try { if (typeof top.document.referrer=="string") { ref=top.document.referrer } } catch(e) {f=3;}'); var url='&fv='+escape(fv)+'&href='+escape(href.substring(0,499)); It crashes in the "var url" line. It doesn't crash if I do any of the following: 1) unwrap the eval 2) remove '&fv='+escape(fv)+ 3) remove '&href='+escape(href.substring(0,499))+ 4) change var href=new String(document.location.href); to var href=document.location.href;

Peter Speck

Comment 8 2010-07-09 13:25:53 PDT

It doesn't crash if the iframe is on the same host (security orgin?) as its parent document. Minimal html which crashes: http://glimmerblocker.org/site/wkbug41948/

Peter Speck

Comment 9 2010-07-11 03:18:12 PDT

Further reduction: now without iframe: http://glimmerblocker.org/site/wkbug41948/noframe.html This html page contains just: abc <script> var href=new String(document.location.href); eval('try { gyfle.gafle.gufle; } catch(e) {}'); var url='abc' + '&href='+escape(href.substring(0,1)); </script> def It doesn't crash if I do any of the following: 1) Changes 1st line to "var href=document.location.href;" 2) Unwraps eval. 3) Removes the "gyfle.gafle.gufle" so it doesn't throw an exception, or replaces it with "throw new Error();". (throw 42; still fails, though). 4) Combines the 2 static strings in "var url=..." 5) Removes substring(), so last line ends with "escape(href);" 6) Adds alert() inside the catch block, i.e. catch(e) { alert(42);}. Anywhere else, and it still crashes. crash confirmed with new nigthly: r63031.

Geoffrey Garen

Comment 10 2010-07-14 13:28:04 PDT

Created attachment 61559 [details] reduction Slightly more reduced.

Geoffrey Garen

Comment 11 2010-07-20 14:53:20 PDT

Created attachment 62116 [details] potential solution Attached a patch that shows the simple-minded way to fix this bug. It breaks fast/js/deep-recursion.html. It was incorrect to add stack shrinking code to exception unwinding. It's possible for a caller to require more stack than a callee, so it's not safe for a callee to shrink the stack to just to satisfy its own needs.

Gavin Barraclough

Comment 12 2010-09-28 14:49:12 PDT

Created attachment 69110 [details] The patch

Geoffrey Garen

Comment 13 2010-09-28 14:55:07 PDT

Comment on attachment 69110 [details] The patch r=me on the code, but patches need regression tests, so you should land the reduction I wrote as a layout test when you land this.

David Tapuska

Comment 14 2010-10-07 06:26:44 PDT

(In reply to comment #13) > (From update of attachment 69110 [details]) > r=me on the code, but patches need regression tests, so you should land the reduction I wrote as a layout test when you land this. we've tried this fix and encounter a crash when on the line: should the line: while (callerCallFrame) { be: while (callerCallFrame && callerCallFrame->codeBlock()) { ?

David Tapuska

Comment 15 2010-10-07 08:03:31 PDT

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 69110 [details] [details]) > > r=me on the code, but patches need regression tests, so you should land the reduction I wrote as a layout test when you land this. > > we've tried this fix and encounter a crash when on the line: > > should the line: > while (callerCallFrame) { > be: > while (callerCallFrame && callerCallFrame->codeBlock()) { > > ? I've modified my local version of the patch to be: Register* maxUsedStack = callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters; CallFrame* callerCallFrame = callFrame->callerFrame(); if (callerCallFrame->hasHostCallFrameFlag()) callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); while (callerCallFrame) { Register* callerMaxUsedStack = callerCallFrame->registers(); if (callerCallFrame->codeBlock()) callerMaxUsedStack += callerCallFrame->codeBlock()->m_numCalleeRegisters; if (callerMaxUsedStack > maxUsedStack) maxUsedStack = callerMaxUsedStack; callerCallFrame = callerCallFrame->callerFrame(); if (callerCallFrame->hasHostCallFrameFlag()) callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); }

Peter Speck

Comment 16 2010-10-07 08:31:11 PDT

(In reply to comment #15) > I've modified my local version of the patch to be: > > Register* maxUsedStack = callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters; > CallFrame* callerCallFrame = callFrame->callerFrame(); > if (callerCallFrame->hasHostCallFrameFlag()) > callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); > while (callerCallFrame) { > Register* callerMaxUsedStack = callerCallFrame->registers(); > if (callerCallFrame->codeBlock()) > callerMaxUsedStack += callerCallFrame->codeBlock()->m_numCalleeRegisters; > if (callerMaxUsedStack > maxUsedStack) > maxUsedStack = callerMaxUsedStack; > callerCallFrame = callerCallFrame->callerFrame(); > if (callerCallFrame->hasHostCallFrameFlag()) > callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); > } It can be simplified to the following, including avoiding a NULL deference when the last callerCallFrame->callerFrame() returns NULL: Register* maxUsedStack = callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters; CallFrame* callerCallFrame = callFrame->callerFrame(); while (callerCallFrame) { if (callerCallFrame->hasHostCallFrameFlag()) callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); Register* callerMaxUsedStack = callerCallFrame->registers(); if (callerCallFrame->codeBlock()) callerMaxUsedStack += callerCallFrame->codeBlock()->m_numCalleeRegisters; if (callerMaxUsedStack > maxUsedStack) maxUsedStack = callerMaxUsedStack; callerCallFrame = callerCallFrame->callerFrame(); } which looks very much like a for-loop.

David Tapuska

Comment 17 2010-10-07 08:46:39 PDT

(In reply to comment #16) Actually I'm not sure it can be. The callFrame could be 0x1 (The host call Frame flag is just the LSB on the address)... indicating it it has a HostCallFrame flag but the callFrame is actually NULL. The code should be as follows to avoid any SEGVs... CallFrame* callerCallFrame = callFrame->callerFrame(); if (callerCallFrame->hasHostCallFrameFlag()) callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); while (callerCallFrame) { Register* callerMaxUsedStack = callerCallFrame->registers(); if (callerCallFrame->codeBlock()) callerMaxUsedStack += callerCallFrame->codeBlock()->m_numCalleeRegisters; if (callerMaxUsedStack > maxUsedStack) maxUsedStack = callerMaxUsedStack; callerCallFrame = callerCallFrame->callerFrame(); if (callerCallFrame && callerCallFrame->hasHostCallFrameFlag()) callerCallFrame = callerCallFrame->removeHostCallFrameFlag(); }

Gavin Barraclough

Comment 18 2010-10-07 09:04:23 PDT

Comment on attachment 69110 [details] The patch Hi David, apologies, I think I may have that last change in my local world. I'll take another look at whether the loop can be simplified when I've fixed the last regression test I'm failing. Clearing review flag due to regression lest failure.

Gavin Barraclough

Comment 19 2010-10-19 12:28:34 PDT

*** Bug 41454 has been marked as a duplicate of this bug. ***

Geoffrey Garen

Comment 20 2010-10-20 11:45:11 PDT

I'll finish this off.

Geoffrey Garen

Comment 21 2010-10-20 13:26:57 PDT

Created attachment 71328 [details] patch

Darin Adler

Comment 22 2010-10-20 13:29:58 PDT

Comment on attachment 71328 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=71328&action=review > JavaScriptCore/interpreter/Interpreter.cpp:680 > + for (CallFrame* callerFrame = callFrame->callerFrame()->removeHostCallFrameFlag(); callerFrame; callerFrame = callerFrame->callerFrame()->removeHostCallFrameFlag()) { This looks a bit like a while rather than for. I always wonder which to use in cases like this one. > JavaScriptCore/interpreter/Interpreter.cpp:687 > + if (callerHighWaterMark <= highWaterMark) > + continue; > + highWaterMark = callerHighWaterMark; You could use max() here instead of an if statement. I think it looks good that way: highWaterMark = max(highWaterMark, callerFrame->registers() + codeBlock->m_numCalleeRegisters);

Geoffrey Garen

Comment 23 2010-10-20 13:54:56 PDT

Committed revision 70174.

WebKit Review Bot

Comment 24 2010-10-20 14:35:59 PDT

http://trac.webkit.org/changeset/70174 might have broken SnowLeopard Intel Release (Tests) The following tests are not passing: http/tests/xmlhttprequest/origin-whitelisting-removal.html

Adam Barth

Comment 25 2010-10-21 01:08:50 PDT

(In reply to comment #24) > http://trac.webkit.org/changeset/70174 might have broken SnowLeopard Intel Release (Tests) > The following tests are not passing: > http/tests/xmlhttprequest/origin-whitelisting-removal.html It's surprising that this is related, but your patch is the only one in the regression range that makes any sense.

Adam Barth

Comment 26 2010-10-21 01:34:17 PDT

I have confirmed that removing your patch fixes the crash. I'm going to roll out your change since it cause a crash.

Adam Barth

Comment 27 2010-10-21 01:36:45 PDT

Committed r70212: <http://trac.webkit.org/changeset/70212>

Adam Barth

Comment 28 2010-10-21 01:37:36 PDT

Rolled out in http://trac.webkit.org/changeset/70212

Geoffrey Garen

Comment 29 2010-10-25 12:23:03 PDT

Hmmm... I asked the sherrifbot to roll this out a few days ago, but I guess it got stuck in the backlog. Thanks for taking care of this, Adam!

Geoffrey Garen

Comment 30 2010-10-26 18:22:03 PDT

Created attachment 71974 [details] patch This time for sure! (Added 0-initialization for the CodeBlock field in CallFrames for host calls.)

Oliver Hunt

Comment 31 2010-10-27 10:42:58 PDT

Comment on attachment 71974 [details] patch r=me

Geoffrey Garen

Comment 32 2010-10-27 11:01:58 PDT

Committed revision 70673.

Andreas Kling

Comment 33 2010-11-22 20:16:13 PST

*** Bug 45457 has been marked as a duplicate of this bug. ***

Balazs Kelemen

Comment 34 2010-11-23 01:34:54 PST

Comment on attachment 71974 [details] patch Clearing flags

Suresh Voruganti

Comment 35 2011-02-15 08:36:44 PST

Please cherry pick the fix for Qtwebkit 2.1.x as this patch fixes the below use case STEPS TO REPRODUCE: 1. Load gazeta.ru 2. Use "full version" link on top or bottom of page if mobile version loaded ACTUAL RESULTS: Browser crashes on loading the full version of page EXPECTED RESULTS: Browser should not crash on page loading. Used QtTestBrowser on N8 to reproduce the bug.

Ademar Reis

Comment 36 2011-02-21 06:16:51 PST

Revision r70673 cherry-picked into qtwebkit-2.1.x with commit 5386b0f <http://gitorious.org/webkit/qtwebkit/commit/5386b0f>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P1

Severity Critical

Classification Unclassified

Version 528+ (Nightly build)

Hardware Mac (Intel)

OS OS X 10.6

Product WebKit

Component JavaScriptCore

Assignee

Geoffrey Garen

Reported

2010-07-09 03:26 PDT

Modified

2011-02-21 06:18 PST History

CC List

11 users Show

URL

http://jp.dk/udland/europa/article2121366.ece

Keywords HasReduction, InRadar, Regression

Duplicates (2)

41454 45457 View as bug list

Depends on

48030

Blocks

Dependencies

tree graph