Bug 183641 - Resource Load Statistics: Add clearing of storage access to WebResourceLoadStatisticsStore::clearInMemory()
Summary: Resource Load Statistics: Add clearing of storage access to WebResourceLoadSt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-14 12:44 PDT by John Wilander
Modified: 2018-03-14 22:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (19.33 KB, patch)
2018-03-14 12:53 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.58 MB, application/zip)
2018-03-14 14:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.30 MB, application/zip)
2018-03-14 14:26 PDT, Build Bot
no flags Details
Patch (22.57 KB, patch)
2018-03-14 16:45 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (19.37 KB, patch)
2018-03-14 16:55 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (22.60 KB, patch)
2018-03-14 17:30 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (23.49 KB, patch)
2018-03-14 18:15 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (23.96 KB, patch)
2018-03-14 18:58 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.66 MB, application/zip)
2018-03-14 20:08 PDT, Build Bot
no flags Details
Patch (19.47 KB, patch)
2018-03-14 20:41 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2018-03-14 12:44:03 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-03-14 12:44:34 PDT
<rdar://problem/38469497>
Comment 2 John Wilander 2018-03-14 12:53:09 PDT
Created attachment 335789 [details]
Patch
Comment 3 Brent Fulgham 2018-03-14 13:02:41 PDT
Comment on attachment 335789 [details]
Patch

Looks good. r=me.
Comment 4 John Wilander 2018-03-14 13:04:48 PDT
(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.
Comment 5 Build Bot 2018-03-14 14:06:07 PDT
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
Comment 6 Build Bot 2018-03-14 14:06:08 PDT
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
Comment 7 Build Bot 2018-03-14 14:26:50 PDT
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
Comment 8 Build Bot 2018-03-14 14:26:52 PDT
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
Comment 9 John Wilander 2018-03-14 16:45:02 PDT
Created attachment 335809 [details]
Patch
Comment 10 John Wilander 2018-03-14 16:45:29 PDT
Trying without the failing test case.
Comment 11 Chris Dumez 2018-03-14 16:49:16 PDT
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);
Comment 12 John Wilander 2018-03-14 16:54:36 PDT
(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.
Comment 13 John Wilander 2018-03-14 16:55:22 PDT
Created attachment 335811 [details]
Patch
Comment 14 John Wilander 2018-03-14 17:30:51 PDT
Created attachment 335814 [details]
Patch
Comment 15 John Wilander 2018-03-14 17:31:48 PDT
if (canSendMessage()) sadly did not fix the issue.
Comment 16 John Wilander 2018-03-14 18:15:20 PDT
It's marked as [ Pass ] in one more place. :(
Comment 17 John Wilander 2018-03-14 18:15:57 PDT
Created attachment 335817 [details]
Patch
Comment 18 John Wilander 2018-03-14 18:58:30 PDT
Created attachment 335822 [details]
Patch
Comment 19 Build Bot 2018-03-14 20:08:44 PDT
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
Comment 20 Build Bot 2018-03-14 20:08:45 PDT
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
Comment 21 Chris Dumez 2018-03-14 20:31:59 PDT
(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
Comment 22 Chris Dumez 2018-03-14 20:34:02 PDT
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().
Comment 23 John Wilander 2018-03-14 20:41:59 PDT
Created attachment 335828 [details]
Patch
Comment 24 John Wilander 2018-03-14 20:43:16 PDT
(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.
Comment 25 WebKit Commit Bot 2018-03-14 22:05:41 PDT
Comment on attachment 335828 [details]
Patch

Clearing flags on attachment: 335828

Committed r229619: <https://trac.webkit.org/changeset/229619>
Comment 26 WebKit Commit Bot 2018-03-14 22:05:43 PDT
All reviewed patches have been landed.  Closing bug.