19853 2008-07-02 02:38:27 -0700 REGRESSION (r34838): Crash when visiting http://www.thewebsiteisdown.com/salesguy.html 2008-07-03 11:14:32 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED http://www.thewebsiteisdown.com/salesguy.html P1 Normal --- 1 yazz zwarich ap dev+webkit ggaren ian.grant zwarich oldest_to_newest 84965 0 yazz 2008-07-02 02:38:27 -0700 Visit http://www.thewebsiteisdown.com/salesguy.html with July 2nd nightly build causes a crash. System running Mac OS X 10.5.4 85049 1 ap 2008-07-03 04:21:51 -0700 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*. 85050 2 ap 2008-07-03 04:27:32 -0700 This didn't crash in r34824, so it's a recent regression - sadly, there was a large gap between nighlies this time. 85052 3 ap 2008-07-03 04:37:59 -0700 *** Bug 19856 has been marked as a duplicate of this bug. *** 85054 4 zwarich 2008-07-03 04:57:38 -0700 This seems like it is caused by r34838: http://trac.webkit.org/changeset/34838 I am testing this now. 85057 5 zwarich 2008-07-03 05:38:57 -0700 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. 85061 6 zwarich 2008-07-03 06:25:46 -0700 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 *** 85063 7 zwarich 2008-07-03 06:27:04 -0700 Actually, this bug is easier to reproduce, so I will swap the duplicate status. Sorry! 85066 8 zwarich 2008-07-03 06:27:25 -0700 *** Bug 19736 has been marked as a duplicate of this bug. *** 85074 9 22064 zwarich 2008-07-03 06:44:53 -0700 Created attachment 22064 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. 85083 10 22064 ap 2008-07-03 07:25:06 -0700 Comment on attachment 22064 Proposed patch So, do we want to fix _NPN_Invoke as well? 85099 11 zwarich 2008-07-03 10:17:26 -0700 (In reply to comment #10) > (From update of attachment 22064 [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? 85100 12 ggaren 2008-07-03 10:20:21 -0700 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). 85101 13 22064 ggaren 2008-07-03 10:21:14 -0700 Comment on attachment 22064 Proposed patch I'll r+ this since it fixes part of the bug. Cameron, can you add comment with a bug #, please, before landing? 85103 14 ap 2008-07-03 10:28:21 -0700 (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? 85106 15 zwarich 2008-07-03 11:00:28 -0700 (In reply to comment #13) > (From update of attachment 22064 [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? 85108 16 zwarich 2008-07-03 11:14:32 -0700 Landed in r34982. 22064 2008-07-03 06:44:53 -0700 2008-07-03 10:21:14 -0700 Proposed patch plugin.diff text/plain 1915 zwarich SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM0OTgw KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjAgQEAKKzIwMDgtMDct MDMgIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1d2F0ZXJsb28uY2E+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDE5ODUzOiBSRUdSRVNT SU9OIChyMzQ4MzgpOiBDcmFzaCB3aGVuIHZpc2l0aW5nIGh0dHA6Ly93d3cudGhld2Vic2l0ZWlz ZG93bi5jb20vc2FsZXNndXkuaHRtbAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTk4NTMKKworICAgICAgICBSZW1vdmUgdGhlIGJvZHkgb2YgX05QTl9T ZXRFeGNlcHRpb24oKSwgYmVjYXVzZSBpdCB3YXMgc2ltcGx5IGNhbGxpbmcKKyAgICAgICAgdGhy b3dFcnJvcigpLCB3aGljaCBzZXRzIGFuIGV4Y2VwdGlvbiBvbiBhbiBFeGVjU3RhdGUgYnV0IGRv ZXMgbm90CisgICAgICAgIGFjdHVhbGx5IGhhbmRsZSBpdC4gVGhlIHByZXNlbmNlIG9mIGFuIGV4 Y2VwdGlvbiBvbiB0aGUgZ2xvYmFsIEV4ZWNTdGF0ZQorICAgICAgICBjYXVzZXMgYXNzZXJ0aW9u cyB0aGF0IHRoZXJlIGlzIG5vIGV4Y2VwdGlvbiBzZXQgb24gdGhhdCBFeGVjU3RhdGUgdG8KKyAg ICAgICAgZmFpbCwgYXMgd2VsbCBhcyBjYXVzaW5nIE1hY2hpbmU6OmV4ZWN1dGUoKSB0byBtaXN0 YWtpbmdseSByZXR1cm4gMCBpbgorICAgICAgICBzb21lIGNhc2VzLCB1bmRlciB0aGUgaW1wcmVz c2lvbiB0aGF0IGl0IGhhcyByYW4gb3V0IG9mIG1lbW9yeS4KKworICAgICAgICAqIGJyaWRnZS9O UF9qc29iamVjdC5jcHA6CisgICAgICAgIChfTlBOX1NldEV4Y2VwdGlvbik6CisKIDIwMDgtMDct MDMgIE1hY2llaiBLYXRhZmlhc3ogIDxtYXRocmlja0BnbWFpbC5jb20+CiAKICAgICAgICAgR3Rr IGJ1aWxkIGZpeCB3aXRoIFNWRyBmaWx0ZXJzIGVuYWJsZWQKSW5kZXg6IGJyaWRnZS9OUF9qc29i amVjdC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gYnJpZGdlL05QX2pzb2JqZWN0LmNwcAkocmV2aXNpb24g MzQ5NzkpCisrKyBicmlkZ2UvTlBfanNvYmplY3QuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNTEs MTggKzM1MSw4IEBAIGJvb2wgX05QTl9IYXNNZXRob2QoTlBQLCBOUE9iamVjdCogbywgTlAKICAg ICByZXR1cm4gZmFsc2U7CiB9CiAKLXZvaWQgX05QTl9TZXRFeGNlcHRpb24oTlBPYmplY3QqIG8s IGNvbnN0IE5QVVRGOCogbWVzc2FnZSkKK3ZvaWQgX05QTl9TZXRFeGNlcHRpb24oTlBPYmplY3Qq LCBjb25zdCBOUFVURjgqKQogewotICAgIGlmIChvLT5fY2xhc3MgPT0gTlBTY3JpcHRPYmplY3RD bGFzcykgewotICAgICAgICBKYXZhU2NyaXB0T2JqZWN0KiBvYmogPSAoSmF2YVNjcmlwdE9iamVj dCopbzsgCi0gICAgICAgIFJvb3RPYmplY3QqIHJvb3RPYmplY3QgPSBvYmotPnJvb3RPYmplY3Q7 Ci0gICAgICAgIGlmICghcm9vdE9iamVjdCB8fCAhcm9vdE9iamVjdC0+aXNWYWxpZCgpKQotICAg ICAgICAgICAgcmV0dXJuOwotCi0gICAgICAgIEV4ZWNTdGF0ZSogZXhlYyA9IHJvb3RPYmplY3Qt Pmdsb2JhbE9iamVjdCgpLT5nbG9iYWxFeGVjKCk7Ci0gICAgICAgIEpTTG9jayBsb2NrKGZhbHNl KTsKLSAgICAgICAgdGhyb3dFcnJvcihleGVjLCBHZW5lcmFsRXJyb3IsIG1lc3NhZ2UpOwotICAg IH0KIH0KIAogYm9vbCBfTlBOX0VudW1lcmF0ZShOUFAsIE5QT2JqZWN0ICpvLCBOUElkZW50aWZp ZXIgKippZGVudGlmaWVyLCB1aW50MzJfdCAqY291bnQpCg==