Visit http://www.thewebsiteisdown.com/salesguy.html with July 2nd nightly build causes a crash. System running Mac OS X 10.5.4
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*.
This didn't crash in r34824, so it's a recent regression - sadly, there was a large gap between nighlies this time.
*** Bug 19856 has been marked as a duplicate of this bug. ***
This seems like it is caused by r34838: http://trac.webkit.org/changeset/34838 I am testing this now.
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.
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 ***
Actually, this bug is easier to reproduce, so I will swap the duplicate status. Sorry!
*** Bug 19736 has been marked as a duplicate of this bug. ***
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 on attachment 22064 [details] Proposed patch So, do we want to fix _NPN_Invoke as well?
(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?
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 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?
(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?
(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?
Landed in r34982.