Bug 41948

Summary: REGRESSION(r60392): Registerfile can be unwound too far following an exception
Product: WebKit Reporter: Peter Speck <speck>
Component: JavaScriptCoreAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Critical CC: abarth, ademar, dave+webkit, eric, ggaren, kbalazs, mrowe, oliver, plaes, suresh.voruganti, webkit.review.bot
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
URL: http://jp.dk/udland/europa/article2121366.ece
Bug Depends on: 48030    
Bug Blocks:    
Attachments:
Description Flags
reduction
none
potential solution
none
The patch
barraclough: review-, ggaren: commit-queue-
patch
none
patch none

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
DetailsFormatted DiffDiff
The patch (3.95 KB, patch)
2010-09-28 14:49 PDT, Gavin Barraclough 		barraclough: review-
ggaren: commit-queue-
DetailsFormatted DiffDiff
patch (4.81 KB, patch)
2010-10-20 13:26 PDT, Geoffrey Garen 		no flags
DetailsFormatted DiffDiff
patch (7.46 KB, patch)
2010-10-26 18:22 PDT, Geoffrey Garen 		no flags
DetailsFormatted DiffDiff
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.