Bug 160325

Summary: Function Call on NULL Object
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, lforschler
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

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.