Resource Load Statistics: Add clearing of storage access to WebResourceLoadStatisticsStore::clearInMemory()

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

WebResourceLoadStatisticsStore::clearInMemory() should clear out all storage access entries in the network process. Right now, we have layout test failures because of one test providing access to another.

[Radar WebKit Bug Importer]

<rdar://problem/38469497>

[John Wilander]

Created attachment 335789 [details]
Patch

[Brent Fulgham]

Comment on attachment 335789 [details]
Patch

Looks good. r=me.

[John Wilander]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 335789 [details]
> Patch
> 
> Looks good. r=me.

Thanks! I'll just wait for the EWS bubbles, then land.

[EWS Watchlist]

Comment on attachment 335789 [details]
Patch

Attachment 335789 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6954405

New failing tests:
http/tests/resourceLoadStatistics/clear-in-memory-and-persistent-store-one-hour.html

[EWS Watchlist]

Created attachment 335795 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335789 [details]
Patch

Attachment 335789 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6954406

New failing tests:
http/tests/resourceLoadStatistics/clear-in-memory-and-persistent-store-one-hour.html

[EWS Watchlist]

Created attachment 335800 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[John Wilander]

Created attachment 335809 [details]
Patch

[John Wilander]

Trying without the failing test case.

[Chris Dumez]

Comment on attachment 335809 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:433
> +    send(Messages::NetworkProcess::RemoveAllStorageAccess(sessionID), 0);

To fix your crash, you may want to try:
if (canSendMessage())
    send(Messages::NetworkProcess::RemoveAllStorageAccess(sessionID), 0);

[John Wilander]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 335809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335809&action=review
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:433
> > +    send(Messages::NetworkProcess::RemoveAllStorageAccess(sessionID), 0);
> 
> To fix your crash, you may want to try:
> if (canSendMessage())
>     send(Messages::NetworkProcess::RemoveAllStorageAccess(sessionID), 0);

Thanks, Chris! I will try this. If it works I should add that check to all the other message sends I have in this proxy.

[John Wilander]

Created attachment 335811 [details]
Patch

[John Wilander]

Created attachment 335814 [details]
Patch

[John Wilander]

if (canSendMessage()) sadly did not fix the issue.

[John Wilander]

It's marked as [ Pass ] in one more place. :(

[John Wilander]

Created attachment 335817 [details]
Patch

[John Wilander]

Created attachment 335822 [details]
Patch

[EWS Watchlist]

Comment on attachment 335822 [details]
Patch

Attachment 335822 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6959540

New failing tests:
http/tests/resourceLoadStatistics/clear-in-memory-and-persistent-store-one-hour.html

[EWS Watchlist]

Created attachment 335825 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

(In reply to John Wilander from comment #15)
> if (canSendMessage()) sadly did not fix the issue.

The crash is a bit different now:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x000000010659abda WebKit::ChildProcessProxy::state() const + 4 (RefPtr.h:88)
1   com.apple.WebKit              	0x0000000106636f33 WebKit::NetworkProcessProxy::removeAllStorageAccess(PAL::SessionID) + 21 (ChildProcessProxy.h:79)
2   com.apple.WebKit              	0x000000010687e615 WebKit::WebsiteDataStore::removeAllStorageAccessHandler() + 139 (HashTable.h:181)
3   com.apple.JavaScriptCore      	0x0000000105d7f78f WTF::RunLoop::performWork() + 447 (Function.h:56)
4   com.apple.JavaScriptCore      	0x0000000105d7f942 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
5   com.apple.CoreFoundation      	0x00007fff86d133e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
6   com.apple.CoreFoundation      	0x00007fff86cf465c __CFRunLoopDoSources0 + 556

[Chris Dumez]

Comment on attachment 335822 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1220
> +        processPool->networkProcess()->removeAllStorageAccess(m_sessionID);

I think chances are that networkProcess() returns null here. Try adding a null check for processPool->networkProcess().

[John Wilander]

Created attachment 335828 [details]
Patch

[John Wilander]

(In reply to Chris Dumez from comment #22)
> Comment on attachment 335822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335822&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1220
> > +        processPool->networkProcess()->removeAllStorageAccess(m_sessionID);
> 
> I think chances are that networkProcess() returns null here. Try adding a
> null check for processPool->networkProcess().

Thanks! New patch up.

[WebKit Commit Bot]

Comment on attachment 335828 [details]
Patch

Clearing flags on attachment: 335828

Committed r229619: <https://trac.webkit.org/changeset/229619>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.