[webkit-reviews] review granted: [Bug 200225] Possible use-after-move under NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated() : [Attachment 375076] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 200225: Possible use-after-move under
NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated()
https://bugs.webkit.org/show_bug.cgi?id=200225

Attachment 375076: Patch

https://bugs.webkit.org/attachment.cgi?id=375076&action=review




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 375076
  --> https://bugs.webkit.org/attachment.cgi?id=375076
Patch

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

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:699
>> +	auto* networkSession =
networkProcess().networkSessionByConnection(connection());
> 
> This seems a bit strange.
> I hope we can remove networkSessionByConnection in the future since a process
can have multiple pages with different sessions.
> It seems also strange that the statistics are not related to some sessionIDs.
> Would it not be safer to keep passing a copy of the statistics to all
sessions? Or pass sessionID with the statistics?

We should probably not pass the statistics to all sessions, as that could be
viewed as a privacy violation if you were servicing multiple network clients
(e.g. an Ephemeral session versus a default session versus a non-default
session). We know that this can happen in use cases like Safari that may have
multiple session to support different tasks unrelated to the main browsing
session.

We should be sending the sessionID with each of these messages. I started a
patch doing this earlier this year but got sidetracked. I should revive it.

I would support doing this in the short term to avoid the issue, and remove
this use of 'networkSessionByConnection' with my proposed update later this
cycle.