Bug 13005 – WebScriptObject +throwException needs NULL check

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dan Waylonis]

When quitting Safari, it's possible for the interpreter to have been torn down, but outstanding vended objects may not have been dealloc'd.  In WebScriptObject, +throwException, before dereferencing "interp", it would be safe and easy to check to see if the value is NULL and if so, return NO.

[Dan Waylonis]

Created attachment 13526 [details]
Patch for 13005 to check for NULL before dereferencing

[David Kilzer (:ddkilzer)]

Comment on attachment 13526 [details]
Patch for 13005 to check for NULL before dereferencing

Dan, don't forget to set the review flag!  :)

Also, this change will need a changelog (WebKitTools/Scripts/prepare-ChangeLog), and if possible an objc unit test.

[Darin Adler]

Comment on attachment 13526 [details]
Patch for 13005 to check for NULL before dereferencing

r=me

[Darin Adler]

Comment on attachment 13526 [details]
Patch for 13005 to check for NULL before dereferencing

Oops! Fix is great, but need a change log entry and, if possible, a regression test.

[Dan Waylonis]

Created attachment 13550 [details]
Patch for 13005 to check for NULL before dereferencing + ChangeLog

I've included the WebCore/ChangeLog modification.  I think that any regression testing would be substantially larger than the patch, so I haven't included a test.

[Darin Adler]

(In reply to comment #5)
> I've included the WebCore/ChangeLog modification.  I think that any regression
> testing would be substantially larger than the patch, so I haven't included a
> test.

I don't understand that rationale. Our tests are often substantially larger than the patches to fix the bug. We definitely want a test if we can make one, because otherwise it's likely that someone in the future will break this.

[Dan Waylonis]

Created attachment 13601 [details]
Patch for 13005 to check for NULL before dereferencing

I've added a test and updated the changelogs.

[Darin Adler]

Comment on attachment 13601 [details]
Patch for 13005 to check for NULL before dereferencing

Looks great. Only some minor problems:

++ (NSString *)webScriptNameForKey:(const char *)key {
++ (BOOL)isKeyExcludedFromWebScript:(const char *)key {
+- (void)dealloc {
+function print(message) {
+function test() {

We put the braces on the next line when defining functions, evein in JavaScript.

The patch has a few tabs in it, and is missing blank lines in the change logs. The change logs say "waylonis" for your name.

I can't tell how the test case detects success. For example, I could take the throwOnDealloc behavior out of the test entirely, and the test would still seem to succeed. Is there something we can do about that? Can we catch the JavaScript exception?

I hate to review- when it's so close, but the tabs at least need to be fixed.

[Dan Waylonis]

Created attachment 13633 [details]
Improved patch to conform with WebKit guidelines

I've corrected the tabs and formatting issues.  As far as the test case goes, unfortunately, it causes a Bus Error with the current version of WebScriptObject.mm when the plugin is dealloc'd.  A successful test does not Bus Error.

[Timothy Hatcher]

This is looking good!

I think this test would be better if JavaScript caught and logged the exception. This would test to make sure JavaScript gets the exception, instead of just having a empty results file.

Expose a method to JavaScript tha throws an exception and call it inside JavaScript inside a a try/catch.

[Dan Waylonis]

Hi Tim,
The problem arises after all of the JavaScript on the page has executed and when the current page/interpreter has been released.  There's no way to catch any exception thrown from JS at this time because there's no JS executing.

The problem is that even though the interpreter has been released, there may still be ObjC objects that have been allocated by the HTML page.  If these objects, in their dealloc method, call [WebScriptObject throwException:@""], the current code will bus error.  My patch checks for the invalid interpreter and returns without trying to dereference a NULL object.

Thanks, Dan

[Timothy Hatcher]

Thanks Dan, I understand now.

[Mark Rowe (bdash)]

Landed in r20296.  Thanks for the patch!