Bug 13005 - WebScriptObject +throwException needs NULL check
Summary: WebScriptObject +throwException needs NULL check
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-03-07 13:56 PST by Dan Waylonis
Modified: 2007-03-18 16:40 PDT (History)
1 user (show)

See Also:


Attachments
Patch for 13005 to check for NULL before dereferencing (591 bytes, patch)
2007-03-07 13:58 PST, Dan Waylonis
darin: review-
Details | Formatted Diff | Diff
Patch for 13005 to check for NULL before dereferencing + ChangeLog (1.09 KB, patch)
2007-03-08 17:33 PST, Dan Waylonis
no flags Details | Formatted Diff | Diff
Patch for 13005 to check for NULL before dereferencing (4.91 KB, patch)
2007-03-12 14:42 PDT, Dan Waylonis
darin: review-
Details | Formatted Diff | Diff
Improved patch to conform with WebKit guidelines (4.98 KB, patch)
2007-03-14 11:39 PDT, Dan Waylonis
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Waylonis 2007-03-07 13:56:04 PST
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.
Comment 1 Dan Waylonis 2007-03-07 13:58:40 PST
Created attachment 13526 [details]
Patch for 13005 to check for NULL before dereferencing
Comment 2 David Kilzer (:ddkilzer) 2007-03-07 16:56:18 PST
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.
Comment 3 Darin Adler 2007-03-08 08:53:49 PST
Comment on attachment 13526 [details]
Patch for 13005 to check for NULL before dereferencing

r=me
Comment 4 Darin Adler 2007-03-08 08:55:07 PST
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.
Comment 5 Dan Waylonis 2007-03-08 17:33:46 PST
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.
Comment 6 Darin Adler 2007-03-09 06:47:27 PST
(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.
Comment 7 Dan Waylonis 2007-03-12 14:42:55 PDT
Created attachment 13601 [details]
Patch for 13005 to check for NULL before dereferencing

I've added a test and updated the changelogs.
Comment 8 Darin Adler 2007-03-12 21:18:30 PDT
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.
Comment 9 Dan Waylonis 2007-03-14 11:39:15 PDT
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.
Comment 10 Timothy Hatcher 2007-03-14 20:04:25 PDT
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.
Comment 11 Dan Waylonis 2007-03-15 11:20:46 PDT
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
Comment 12 Timothy Hatcher 2007-03-16 07:59:16 PDT
Thanks Dan, I understand now.
Comment 13 Mark Rowe (bdash) 2007-03-18 16:40:00 PDT
Landed in r20296.  Thanks for the patch!