Bug 200225 - Possible use-after-move under NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated()
Summary: Possible use-after-move under NetworkConnectionToWebProcess::resourceLoadStat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 196407
  Show dependency treegraph
 
Reported: 2019-07-29 08:24 PDT by Chris Dumez
Modified: 2019-07-29 09:43 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.50 KB, patch)
2019-07-29 08:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-07-29 08:24:57 PDT
Possible use-after-move under NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(). Was WTFMove()-ing the method parameter inside of a loop.
Comment 1 Chris Dumez 2019-07-29 08:27:52 PDT
Created attachment 375076 [details]
Patch
Comment 2 youenn fablet 2019-07-29 08:52:48 PDT
Comment on attachment 375076 [details]
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?
Comment 3 Brent Fulgham 2019-07-29 09:07:38 PDT
Comment on attachment 375076 [details]
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.
Comment 4 WebKit Commit Bot 2019-07-29 09:42:29 PDT
Comment on attachment 375076 [details]
Patch

Clearing flags on attachment: 375076

Committed r247905: <https://trac.webkit.org/changeset/247905>
Comment 5 WebKit Commit Bot 2019-07-29 09:42:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-07-29 09:43:18 PDT
<rdar://problem/53665272>