Bug 19853 – REGRESSION (r34838): Crash when visiting http://www.thewebsiteisdown.com/salesguy.html

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yazz D. Atlas]

Visit http://www.thewebsiteisdown.com/salesguy.html with July 2nd nightly build causes a crash.  System running  Mac OS X 10.5.4

[Alexey Proskuryakov]

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*.

[Alexey Proskuryakov]

This didn't crash in r34824, so it's a recent regression - sadly, there was a large gap between nighlies this time.

[Alexey Proskuryakov]

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

[Cameron Zwarich (cpst)]

This seems like it is caused by r34838:

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

I am testing this now.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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 ***

[Cameron Zwarich (cpst)]

Actually, this bug is easier to reproduce, so I will swap the duplicate status. Sorry!

[Cameron Zwarich (cpst)]

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

[Cameron Zwarich (cpst)]

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.

[Alexey Proskuryakov]

Comment on attachment 22064 [details]
Proposed patch

So, do we want to fix _NPN_Invoke as well?

[Cameron Zwarich (cpst)]

(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?

[Geoffrey Garen]

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).

[Geoffrey Garen]

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?

[Alexey Proskuryakov]

(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?

[Cameron Zwarich (cpst)]

(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?

[Cameron Zwarich (cpst)]

Landed in r34982.