Bug 19853 - REGRESSION (r34838): Crash when visiting http://www.thewebsiteisdown.com/salesguy.html
Summary: REGRESSION (r34838): Crash when visiting http://www.thewebsiteisdown.com/sale...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Cameron Zwarich (cpst)
URL: http://www.thewebsiteisdown.com/sales...
Keywords:
: 19736 19856 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-07-02 02:38 PDT by Yazz D. Atlas
Modified: 2008-07-03 11:14 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (1.87 KB, patch)
2008-07-03 06:44 PDT, Cameron Zwarich (cpst)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yazz D. Atlas 2008-07-02 02:38:27 PDT
Visit http://www.thewebsiteisdown.com/salesguy.html with July 2nd nightly build causes a crash.  System running  Mac OS X 10.5.4
Comment 1 Alexey Proskuryakov 2008-07-03 04:21:51 PDT
Looks like there are two issues here: (1) Machine::privateExecute has started returning 0, and (2), _NPN_Invoke not being prepared to see 0 as the returned JSValue*.
Comment 2 Alexey Proskuryakov 2008-07-03 04:27:32 PDT
This didn't crash in r34824, so it's a recent regression - sadly, there was a large gap between nighlies this time.
Comment 3 Alexey Proskuryakov 2008-07-03 04:37:59 PDT
*** Bug 19856 has been marked as a duplicate of this bug. ***
Comment 4 Cameron Zwarich (cpst) 2008-07-03 04:57:38 PDT
This seems like it is caused by r34838:

http://trac.webkit.org/changeset/34838

I am testing this now.
Comment 5 Cameron Zwarich (cpst) 2008-07-03 05:38:57 PDT
I can confirm that the crash described in this bug is caused by r34838. However, in r34837, both sites still cause assertion failures, thanks to bug 19736. I have a fix for that bug, but I got a bit sidetracked writing the plugin test code.

I am going to remove the assignment of this to myself, because Geoff is likely the best person to fix it.
Comment 6 Cameron Zwarich (cpst) 2008-07-03 06:25:46 PDT
As Alexey mentioned, the problem is _NPN_Invoke not being able to handle a 0 return value from Machine::execute(). The particular 0 in question is coming from

    Register* r = slideRegisterWindowForCall(exec, newCodeBlock, &m_registerFile, m_registerFile.base(), callFrame, argv, argc, *exception);
    if (*exception) {
        m_registerFile.shrink(oldSize);
        return 0;
    }

However, the call to slideRegisterWindowForCall() does not set an exception, so the exception must be sitting around from before. Indeed, it was set by _NPN_SetException(). The fix for bug 19736 is to simply remove the body of _NPN_SetException() because it is completely incorrect and not doing anything useful at the moment. The question of how to properly handle exceptions from the NPAPI is somewhat thorny, because of compatibility issues.

I'm marking this as a duplicate.

*** This bug has been marked as a duplicate of 19736 ***
Comment 7 Cameron Zwarich (cpst) 2008-07-03 06:27:04 PDT
Actually, this bug is easier to reproduce, so I will swap the duplicate status. Sorry!
Comment 8 Cameron Zwarich (cpst) 2008-07-03 06:27:25 PDT
*** Bug 19736 has been marked as a duplicate of this bug. ***
Comment 9 Cameron Zwarich (cpst) 2008-07-03 06:44:53 PDT
Created attachment 22064 [details]
Proposed patch

Here is a patch that fixes the bug. I should be able to reproduce this in a layout test by adding a test to the test plugin, but I might not be able to get around to it until later in the day.
Comment 10 Alexey Proskuryakov 2008-07-03 07:25:06 PDT
Comment on attachment 22064 [details]
Proposed patch

So, do we want to fix _NPN_Invoke as well?
Comment 11 Cameron Zwarich (cpst) 2008-07-03 10:17:26 PDT
(In reply to comment #10)
> (From update of attachment 22064 [details] [edit])
> So, do we want to fix _NPN_Invoke as well?

A 0 return value from Machine::execute() means complete failure. Why do we want to continue execution?
Comment 12 Geoffrey Garen 2008-07-03 10:20:21 PDT
There are actually a few places in our code that are not capable of handling a 0 JSValue*. It would be nice to fix all our code so that it didn't try to use a JSValue* returned from a function that threw an exception. However, in the short term, I think we should just change Machine::privateExecute to return jsNull() in exception cases instead of 0 (in addition to the patch Cameron posted).
Comment 13 Geoffrey Garen 2008-07-03 10:21:14 PDT
Comment on attachment 22064 [details]
Proposed patch

I'll r+ this since it fixes part of the bug. Cameron, can you add  comment with a bug #, please, before landing?
Comment 14 Alexey Proskuryakov 2008-07-03 10:28:21 PDT
(In reply to comment #11)
> A 0 return value from Machine::execute() means complete failure. Why do we want
> to continue execution?

Getting JS stack overflow doesn't mean WebKit should crash, does it?
Comment 15 Cameron Zwarich (cpst) 2008-07-03 11:00:28 PDT
(In reply to comment #13)
> (From update of attachment 22064 [details] [edit])
> I'll r+ this since it fixes part of the bug. Cameron, can you add  comment with
> a bug #, please, before landing?

There's already one in the ChangeLog for this bug, so I imagine you mean a bug # for implementing _NPN_SetException() correctly?
Comment 16 Cameron Zwarich (cpst) 2008-07-03 11:14:32 PDT
Landed in r34982.