Bug 160325 - Function Call on NULL Object
Summary: Function Call on NULL Object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-28 16:10 PDT by Jonathan Bedard
Modified: 2016-08-01 10:51 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.31 KB, patch)
2016-07-28 16:15 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2016-07-29 09:15 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2016-07-28 16:10:17 PDT
In InjectedBundle.cpp, the 'reset' signal is received at the beginning of a test run.  In this reset, all web notifications are removed from the test runner.  However, the first reset message is received before the test runner has been instantiated.
Comment 1 Jonathan Bedard 2016-07-28 16:11:51 PDT
Note that this problem was only revealed with UBSan since removeAllWebNotifications() does not actually need the test runner to be instantiated to function.

Another possible solution is to make this function a static one, but that would pollute the global namespace and may not be desired.
Comment 2 Jonathan Bedard 2016-07-28 16:15:48 PDT
Created attachment 284832 [details]
Patch
Comment 3 Darin Adler 2016-07-28 16:33:14 PDT
(In reply to comment #1)
> Another possible solution is to make this function a static one, but that
> would pollute the global namespace and may not be desired.

We should make it a static member function. That won’t pollute any namespaces at all.
Comment 4 Jonathan Bedard 2016-07-29 09:15:10 PDT
Created attachment 284867 [details]
Patch
Comment 5 Jonathan Bedard 2016-07-29 09:17:26 PDT
I made all functions in this family static, since none of them actually need the object to work.

As a larger architectural question: why aren't more functions in the TestRunner static?  Inspection of the code seems to indicate that most functions in the TestRunner could be declared as static.
Comment 6 Darin Adler 2016-08-01 10:31:43 PDT
(In reply to comment #5)
> As a larger architectural question: why aren't more functions in the
> TestRunner static?  Inspection of the code seems to indicate that most
> functions in the TestRunner could be declared as static.

TestRunner is primarily an object that exposes bindings to be called from JavaScript using the JavaScript testRunner object exposed to tests. People writing these functions probably don’t think about which ones do and don’t need the state; it would be fine, although not particularly important, to change any that can be static member functions to be defined that way.
Comment 7 WebKit Commit Bot 2016-08-01 10:51:20 PDT
Comment on attachment 284867 [details]
Patch

Clearing flags on attachment: 284867

Committed r203975: <http://trac.webkit.org/changeset/203975>
Comment 8 WebKit Commit Bot 2016-08-01 10:51:24 PDT
All reviewed patches have been landed.  Closing bug.