Bug 198814 – WebResourceLoadStatisticsStore should not use its network session if invalidated

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

WebResourceLoadStatisticsStore should not use its network session if invalidated

[youenn fablet]

Created attachment 372002 [details]
Patch

[youenn fablet]

<rdar://problem/49608908>

[Geoffrey Garen]

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?

[youenn fablet]

(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.

[youenn fablet]

Created attachment 372009 [details]
Patch

[John Wilander]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 372135 [details]
Patch

[Geoffrey Garen]

Comment on attachment 372135 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 372135 [details]
Patch

Clearing flags on attachment: 372135

Committed r246449: <https://trac.webkit.org/changeset/246449>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.