WebResourceLoadStatisticsStore should not use its network session if invalidated
Created attachment 372002 [details] Patch
<rdar://problem/49608908>
Comment on attachment 372002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372002&action=review > Source/WebKit/ChangeLog:4 > + WebResourceLoadStatisticsStore should not use its network session if invalidated > + https://bugs.webkit.org/show_bug.cgi?id=198814 What kind of bug does this fix? Can we test it? > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1027 > +void WebResourceLoadStatisticsStore::invalidateAndCancel() > +{ > + m_networkSession = nullptr; > +} Is the purpose here to avoid further updates, or to guard against use after free? If use after free, I'd suggest WeakPtr instead. > Source/WebKit/NetworkProcess/NetworkSession.cpp:104 > + if (m_resourceLoadStatistics) > + m_resourceLoadStatistics->invalidateAndCancel(); For whatever updates we were receiving before this patch, can we add assertions that we aren't updated after being invalidated?
(In reply to Geoffrey Garen from comment #3) > Comment on attachment 372002 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372002&action=review > > > Source/WebKit/ChangeLog:4 > > + WebResourceLoadStatisticsStore should not use its network session if invalidated > > + https://bugs.webkit.org/show_bug.cgi?id=198814 > > What kind of bug does this fix? Can we test it? The issue as I understand it is the following: - NetworkSession is invalidated so is removed from the network process map. Its storage session is also removed. - NetworkSession stays alive as it is refed by other objects. - Its WebResourceLoadStatisticsStore is still processing some tasks, for instance in a background thread and will hop back to the main thread - When in the main thread, WebResourceLoadStatisticsStore tries to get its storage session from its NetworkSession. Since NetworkSession is invalidated, there is no more a storage session and we are hitting a RELEASE_ASSERT. Testing this is probably inherently racy. I think the idea would be to: 1. Create a network session ID 2. Trigger some WebResourceLoadStatisticsStore background processing 3. Delete the network session ID as soon as possible. Not sure about how to trigger step 2, probably Brent or John would know. > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1027 > > +void WebResourceLoadStatisticsStore::invalidateAndCancel() > > +{ > > + m_networkSession = nullptr; > > +} > > Is the purpose here to avoid further updates, or to guard against use after > free? If use after free, I'd suggest WeakPtr instead. The idea is to avoid further updates, given its storage session is gone. m_networkSession is already a WeakPtr. Although this patch makes WeakPtr unnecessary, it seems good to keep it as a WeakPtr instead of using a raw pointer. > > Source/WebKit/NetworkProcess/NetworkSession.cpp:104 > > + if (m_resourceLoadStatistics) > > + m_resourceLoadStatistics->invalidateAndCancel(); > > For whatever updates we were receiving before this patch, can we add > assertions that we aren't updated after being invalidated? Good idea. While I do not think this can happen now, this could happen in the future.
Created attachment 372009 [details] Patch
Comment on attachment 372009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372009&action=review One way to test this would be to schedule a task with a fixed timeout, invalidate, and make sure we exit gracefully. It could be a dummy task for testing that uses the ITP background thread logic. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1024 > +void WebResourceLoadStatisticsStore::invalidateAndCancel() "AndCancel" sounds a little too open ended. Cancel what? Could we go with something like "AndCancelBackgroundTasks"? > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1026 > + m_networkSession = nullptr; Setting this to null automatically cancels everything?
Comment on attachment 372009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372009&action=review I think this looks like a reasonable change. However, It's not marked for review, so I haven't r+'d it. >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1026 >> + m_networkSession = nullptr; > > Setting this to null automatically cancels everything? m_networkSession is just a WeakPtr, so I don't think anything will get cancelled. However, it will prevent all of the methods in this class to return early without doing work.
Comment on attachment 372002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372002&action=review >>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1027 >>> +} >> >> Is the purpose here to avoid further updates, or to guard against use after free? If use after free, I'd suggest WeakPtr instead. > > The idea is to avoid further updates, given its storage session is gone. > > m_networkSession is already a WeakPtr. Although this patch makes WeakPtr unnecessary, it seems good to keep it as a WeakPtr instead of using a raw pointer. Agreed. Nulling it out seems fine. If we happen to know that m_networkSession is soon to be invalid, clearing it ourselves seems like a good idea.
(In reply to Brent Fulgham from comment #7) > Comment on attachment 372009 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372009&action=review > > I think this looks like a reasonable change. However, It's not marked for > review, so I haven't r+'d it. I plan to write a test that will (flakily) cover this case. > >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1026 > >> + m_networkSession = nullptr; > > > > Setting this to null automatically cancels everything? > > m_networkSession is just a WeakPtr, so I don't think anything will get > cancelled. However, it will prevent all of the methods in this class to > return early without doing work. Right, it is cancelling all actions related to NetworkSession/NetworkSessionStorage, all IPC... It is not cancelling postTask/completion handler things. It is also not cancelling actions related to memory/persistent store which as I see it is probably fine.
Created attachment 372135 [details] Patch
Comment on attachment 372135 [details] Patch r=me
Comment on attachment 372135 [details] Patch Clearing flags on attachment: 372135 Committed r246449: <https://trac.webkit.org/changeset/246449>
All reviewed patches have been landed. Closing bug.