ResourceLoadStatisticsClassifierCocoa::singletonPredictionModel() should check the return value of storagePath() before attempting to load

[John Wilander]

ResourceLoadStatisticsClassifierCocoa::singletonPredictionModel() should check the return value of storagePath() before attempting to load the model file.

[John Wilander]

rdar://problem/32671352

[John Wilander]

Created attachment 316906 [details]
Patch

[Build Bot]

Comment on attachment 316906 [details]
Patch

Attachment 316906 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4240177

New failing tests:
fast/dom/Window/HTMLBodyElement-window-eventListener-attributes.html

[Build Bot]

Created attachment 316963 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[David Kilzer (:ddkilzer)]

Comment on attachment 316906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316906&action=review

r=me since this resolves a thread-safety issue when creating corePredictionModel (since thread-safe C++ statics are disabled in WebKit), but I'm not sure it will resolve all potential causes of this crash.

Need to check if the CorePrediction API being used is also thread-safe when using the same model on more than one thread at the same time.

> Source/WebKit/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:111
> +        if (path.isEmpty())
> +            return;

You should add some WTFLogAlways() logging output here, too, if it's unexpected that this path will ever be empty.

[David Kilzer (:ddkilzer)]

Comment on attachment 316906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316906&action=review

>> Source/WebKit/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:111
>> +            return;
> 
> You should add some WTFLogAlways() logging output here, too, if it's unexpected that this path will ever be empty.

Oops, John correctly pointed out this will get logged below on Line 119 if it fails, so no need to make a change here.

[John Wilander]

Comment on attachment 316906 [details]
Patch

Failed layout test looks unrelated. Successfully ran it 50 times (with guard malloc).

Thanks for the review, Dave!

[WebKit Commit Bot]

Comment on attachment 316906 [details]
Patch

Clearing flags on attachment: 316906

Committed r220136: <http://trac.webkit.org/changeset/220136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.