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
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
This was caused by <http://trac.webkit.org/changeset/60392>.
<rdar://problem/8174724>
Mark, can you reproduce this crash? I couldn't.
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.
Geoff, yes, I can reproduce this 100% of the time.
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;
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/
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.
Created attachment 61559 [details] reduction Slightly more reduced.
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.
Created attachment 69110 [details] The patch
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.
(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()) { ?
(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(); }
(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.
(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(); }
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.
*** Bug 41454 has been marked as a duplicate of this bug. ***
I'll finish this off.
Created attachment 71328 [details] patch
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);
Committed revision 70174.
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
(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.
I have confirmed that removing your patch fixes the crash. I'm going to roll out your change since it cause a crash.
Committed r70212: <http://trac.webkit.org/changeset/70212>
Rolled out in http://trac.webkit.org/changeset/70212
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!
Created attachment 71974 [details] patch This time for sure! (Added 0-initialization for the CodeBlock field in CallFrames for host calls.)
Comment on attachment 71974 [details] patch r=me
Committed revision 70673.
*** Bug 45457 has been marked as a duplicate of this bug. ***
Comment on attachment 71974 [details] patch Clearing flags
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.
Revision r70673 cherry-picked into qtwebkit-2.1.x with commit 5386b0f <http://gitorious.org/webkit/qtwebkit/commit/5386b0f>