Bug 41948 - REGRESSION(r60392): Registerfile can be unwound too far following an exception
Summary: REGRESSION(r60392): Registerfile can be unwound too far following an exception
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P1 Critical
Assignee: Geoffrey Garen
URL: http://jp.dk/udland/europa/article212...
Keywords: HasReduction, InRadar, Regression
: 41454 45457 (view as bug list)
Depends on: 48030
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-09 03:26 PDT by Peter Speck
Modified: 2011-02-21 06:18 PST (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Speck 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
Comment 1 Peter Speck 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
Comment 2 Mark Rowe (bdash) 2010-07-09 05:01:00 PDT
This was caused by <http://trac.webkit.org/changeset/60392>.
Comment 3 Mark Rowe (bdash) 2010-07-09 05:01:27 PDT
<rdar://problem/8174724>
Comment 4 Geoffrey Garen 2010-07-09 10:58:18 PDT
Mark, can you reproduce this crash? I couldn't.
Comment 5 Peter Speck 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.
Comment 6 Mark Rowe (bdash) 2010-07-09 12:56:14 PDT
Geoff, yes, I can reproduce this 100% of the time.
Comment 7 Peter Speck 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;
Comment 8 Peter Speck 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/
Comment 9 Peter Speck 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.
Comment 10 Geoffrey Garen 2010-07-14 13:28:04 PDT
Created attachment 61559 [details]
reduction

Slightly more reduced.
Comment 11 Geoffrey Garen 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.
Comment 12 Gavin Barraclough 2010-09-28 14:49:12 PDT
Created attachment 69110 [details]
The patch
Comment 13 Geoffrey Garen 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.
Comment 14 David Tapuska 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()) { 

?
Comment 15 David Tapuska 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();
    }
Comment 16 Peter Speck 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.
Comment 17 David Tapuska 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();
    }
Comment 18 Gavin Barraclough 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.
Comment 19 Gavin Barraclough 2010-10-19 12:28:34 PDT
*** Bug 41454 has been marked as a duplicate of this bug. ***
Comment 20 Geoffrey Garen 2010-10-20 11:45:11 PDT
I'll finish this off.
Comment 21 Geoffrey Garen 2010-10-20 13:26:57 PDT
Created attachment 71328 [details]
patch
Comment 22 Darin Adler 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);
Comment 23 Geoffrey Garen 2010-10-20 13:54:56 PDT
Committed revision 70174.
Comment 24 WebKit Review Bot 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
Comment 25 Adam Barth 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.
Comment 26 Adam Barth 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.
Comment 27 Adam Barth 2010-10-21 01:36:45 PDT
Committed r70212: <http://trac.webkit.org/changeset/70212>
Comment 28 Adam Barth 2010-10-21 01:37:36 PDT
Rolled out in http://trac.webkit.org/changeset/70212
Comment 29 Geoffrey Garen 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!
Comment 30 Geoffrey Garen 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.)
Comment 31 Oliver Hunt 2010-10-27 10:42:58 PDT
Comment on attachment 71974 [details]
patch

r=me
Comment 32 Geoffrey Garen 2010-10-27 11:01:58 PDT
Committed revision 70673.
Comment 33 Andreas Kling 2010-11-22 20:16:13 PST
*** Bug 45457 has been marked as a duplicate of this bug. ***
Comment 34 Balazs Kelemen 2010-11-23 01:34:54 PST
Comment on attachment 71974 [details]
patch

Clearing flags
Comment 35 Suresh Voruganti 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.
Comment 36 Ademar Reis 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>