Bug 198923 – Support ITP on a per-session basis

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

ITP should be controllable on a per-session basis, for example to allow different ITP behavior for ephemeral sessions. Currently there is only a single global flag controlling behavior.

[Radar WebKit Bug Importer]

<rdar://problem/54213854>

[Kate Cheney]

Created attachment 376954 [details]
Patch

[Daniel Bates]

Comment on attachment 376954 [details]
Patch

This change looks good. In my opinion this patch would benefit by defining a type alias for HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>> because:

1. It’s long to type out this type.
2. Less chance for misspelling with a shorter type => less re-compiles => increase developer productivity.
3. The alias’s name could be more succinct/canonical and easier to refer to both in new code and conversations about this code.
4. Allow us to re-define this type later in one central place, reducing the amount of future code changes.

[youenn fablet]

Comment on attachment 376954 [details]
Patch

Patch looks almost ready to me as well.
A couple of small improvements below.
The main change to consider is whether we really need to transfer a map<sessionID, statistics> from WebProcess to NetworkProcess.
Passing a Vector would probably be more efficient, which could impact the notification callback signature.

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:57
> +void ResourceLoadObserver::setStatisticsUpdatedCallback(WTF::Function<void(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>)>&& notificationCallback)

No need for WTF:: here and in setRequestStorageAccessUnderOpenerCallback below.

> Source/WebCore/loader/ResourceLoadObserver.cpp:365
> +        addResult.iterator->value = makeUnique<HashMap<RegistrableDomain, ResourceLoadStatistics>>();

You can use m_perSessionResourceStatisticsMap.ensure here.

> Source/WebCore/loader/ResourceLoadObserver.cpp:400
> +            statistics.uncheckedAppend(WTFMove(statistic));

You move statistic but statistic will stay in m_perSessionResourceStatisticsMap.
Is it by design?
This might be okish here but maybe we can improve things.
1. Copy instead of move although I guess this is not what we want here.
2. Clear m_perSessionResourceStatisticsMap. Probably fine but maybe this is not done for perf reasons?
3. Add a take method to ResourceLoadStatistics and call statistics.uncheckedAppend(statistic.take()) here. The take method will be responsible to move statistic internals and keep it in a proper state.

> Source/WebCore/loader/ResourceLoadObserver.h:76
> +    WEBCORE_EXPORT void setStatisticsUpdatedCallback(WTF::Function<void(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>)>&&);

No need for WTF::

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:715
> +void NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>&& statistics)

There is no real need to have a HashMap here, a Vector<struct {SessionID, ResourceLoadStatistics}> would be good enough and probably more efficient.

> Source/WebKit/WebProcess/WebProcess.cpp:221
>          ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::ResourceLoadStatisticsUpdated(WTFMove(statistics)), 0);

I believe there is no need to WTFMove(statistics) here

[Kate Cheney]

(In reply to youenn fablet from comment #4)
> Comment on attachment 376954 [details]
> Patch
> 
> Patch looks almost ready to me as well.
> A couple of small improvements below.
> The main change to consider is whether we really need to transfer a
> map<sessionID, statistics> from WebProcess to NetworkProcess.
> Passing a Vector would probably be more efficient, which could impact the
> notification callback signature.

Thanks for the feedback! I'm updating the patch now and had some followup questions.

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376954&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:57
> > +void ResourceLoadObserver::setStatisticsUpdatedCallback(WTF::Function<void(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>)>&& notificationCallback)
> 
> No need for WTF:: here and in setRequestStorageAccessUnderOpenerCallback
> below.

Yes, you're right -- thanks.

> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:365
> > +        addResult.iterator->value = makeUnique<HashMap<RegistrableDomain, ResourceLoadStatistics>>();
> 
> You can use m_perSessionResourceStatisticsMap.ensure here.
> 

I think add is more readable. However, if there is a performance reason to use ensure I will happily change it.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:400
> > +            statistics.uncheckedAppend(WTFMove(statistic));
> 
> You move statistic but statistic will stay in
> m_perSessionResourceStatisticsMap.
> Is it by design?
> This might be okish here but maybe we can improve things.
> 1. Copy instead of move although I guess this is not what we want here.
> 2. Clear m_perSessionResourceStatisticsMap. Probably fine but maybe this is
> not done for perf reasons?
> 3. Add a take method to ResourceLoadStatistics and call
> statistics.uncheckedAppend(statistic.take()) here. The take method will be
> responsible to move statistic internals and keep it in a proper state.
> 

Still looking into this... I agree it is weird. Thanks for the suggestions! 

> > Source/WebCore/loader/ResourceLoadObserver.h:76
> > +    WEBCORE_EXPORT void setStatisticsUpdatedCallback(WTF::Function<void(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>)>&&);
> 
> No need for WTF::
> 

Got it. 

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:715
> > +void NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>&& statistics)
> 
> There is no real need to have a HashMap here, a Vector<struct {SessionID,
> ResourceLoadStatistics}> would be good enough and probably more efficient.
> 

Sounds good. Would a Vector of tuples or pairs be easier than declaring a new struct?

> > Source/WebKit/WebProcess/WebProcess.cpp:221
> >          ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::ResourceLoadStatisticsUpdated(WTFMove(statistics)), 0);
> 
> I believe there is no need to WTFMove(statistics) here

Okay. I'm not familiar with why -- can we be sure that statistics won't be copied without the WTFMove?

[Chris Dumez]

Comment on attachment 376954 [details]
Patch

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

>>> Source/WebCore/loader/ResourceLoadObserver.cpp:365
>>> +        addResult.iterator->value = makeUnique<HashMap<RegistrableDomain, ResourceLoadStatistics>>();
>> 
>> You can use m_perSessionResourceStatisticsMap.ensure here.
> 
> I think add is more readable. However, if there is a performance reason to use ensure I will happily change it.

ah ah, I made the same comment than Youenn offline. It feel very weird to add nullptr and then overwrite the value with something else, now that we have ensure(), not to mention this is unnecessary work. I do believe ensure() is faster for this reason, but I have not measured.

> Source/WebCore/loader/ResourceLoadObserver.cpp:380
> +    auto mapIter = m_perSessionResourceStatisticsMap.find(sessionID);

I think this would look better with a get() than a find().

> Source/WebCore/loader/ResourceLoadObserver.cpp:-390
> -    m_resourceStatisticsMap.clear();

Is it me or is m_resourceStatisticsMap no longer cleared now? This seems like a bug.

>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:715
>>> +void NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(HashMap<PAL::SessionID, Vector<ResourceLoadStatistics>>&& statistics)
>> 
>> There is no real need to have a HashMap here, a Vector<struct {SessionID, ResourceLoadStatistics}> would be good enough and probably more efficient.
> 
> Sounds good. Would a Vector of tuples or pairs be easier than declaring a new struct?

Yes, a vector of std::pair would be fine IMO.

>>> Source/WebKit/WebProcess/WebProcess.cpp:221
>>>          ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::ResourceLoadStatisticsUpdated(WTFMove(statistics)), 0);
>> 
>> I believe there is no need to WTFMove(statistics) here
> 
> Okay. I'm not familiar with why -- can we be sure that statistics won't be copied without the WTFMove?

The WTFMove() does not hurt here, it also does not do anything. I would just keep it personally.

[youenn fablet]

> >> I believe there is no need to WTFMove(statistics) here
> > 
> > Okay. I'm not familiar with why -- can we be sure that statistics won't be copied without the WTFMove?

The IPC message is taking a const reference so the WTFMove has no runtime impact.

> 
> The WTFMove() does not hurt here, it also does not do anything. I would just
> keep it personally.

Agreed it has no runtime impact but it is misleading when reading the code.
For instance, it gives the false impression that the callback should be given an r-value. In practice, a const ref is sufficient.
I would remove it.

[youenn fablet]

> Sounds good. Would a Vector of tuples or pairs be easier than declaring a
> new struct?

I usually personally prefer a struct over a pair as I find the code easier to read.
In this particular case though, the struct is mainly for IPC and we would need to add encode/decode for these routines.
This is given for free with a pair so a pair is probably the way to go.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> > >> I believe there is no need to WTFMove(statistics) here
> > > 
> > > Okay. I'm not familiar with why -- can we be sure that statistics won't be copied without the WTFMove?
> 
> The IPC message is taking a const reference so the WTFMove has no runtime
> impact.
> 
> > 
> > The WTFMove() does not hurt here, it also does not do anything. I would just
> > keep it personally.
> 
> Agreed it has no runtime impact but it is misleading when reading the code.
> For instance, it gives the false impression that the callback should be
> given an r-value. In practice, a const ref is sufficient.
> I would remove it.

I don't feel as strongly as you so whatever, but to me WTFMove() is merely a cast indicating to the callee that it may take the value because the current method does not need it anymore. The callee may or may not to do now (or in the future). The point here is that the caller definitely does not need that statistics anymore, it is merely forwarding them to the IPC so I feel WTFMove() is appropriate, even if the particular callee does not leverage the r-value reference right now.

[Kate Cheney]

Created attachment 377075 [details]
Patch

[Chris Dumez]

Comment on attachment 377075 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:57
> +void ResourceLoadObserver::setStatisticsUpdatedCallback(Function<void(ResourceLoadObserver::PerSessionResourceLoadData)>&& notificationCallback)

ResourceLoadObserver:: should not be needed.

> Source/WebCore/loader/ResourceLoadObserver.cpp:380
> +    auto resourceStatisticsByDomain = m_perSessionResourceStatisticsMap.get(sessionID);

auto* to make it clear it is a raw pointer.

> Source/WebCore/loader/ResourceLoadObserver.cpp:391
> +ResourceLoadObserver::PerSessionResourceLoadData ResourceLoadObserver::takeStatistics()

I personally like:
auto ResourceLoadObserver::takeStatistics() -> PerSessionResourceLoadData

so that ResourceLoadObserver:: scope is not needed, but your call.

> Source/WebCore/loader/ResourceLoadObserver.cpp:393
> +    ResourceLoadObserver::PerSessionResourceLoadData perSessionStatistics;

ResourceLoadObserver:: should not be needed.

> Source/WebCore/loader/ResourceLoadObserver.cpp:402
> +        perSessionStatistics.uncheckedAppend(std::make_pair(iter.key, WTFMove(statistics)));

uncheckedAppend() is unsafe here since you did not reserve capacity. This should just be an append().

> Source/WebCore/loader/ResourceLoadObserver.h:62
> +    typedef Vector<std::pair<PAL::SessionID, Vector<ResourceLoadStatistics>>> PerSessionResourceLoadData;

I think we like using statements better nowadays:
using PerSessionResourceLoadData = Vector<std::pair<PAL::SessionID, Vector<ResourceLoadStatistics>>>;

> Source/WebCore/loader/ResourceLoadObserver.h:77
> +    WEBCORE_EXPORT void setStatisticsUpdatedCallback(Function<void(PerSessionResourceLoadData)>&&);

PerSessionResourceLoadData&&, not a copy.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:226
> +        void resourceLoadStatisticsUpdated(WebCore::ResourceLoadObserver::PerSessionResourceLoadData&&);

indentation problem

> Source/WebKit/WebProcess/WebProcess.cpp:220
> +    ResourceLoadObserver::shared().setStatisticsUpdatedCallback([this] (ResourceLoadObserver::PerSessionResourceLoadData&& statistics) {

I believe auto&& would work here.

[Kate Cheney]

Created attachment 377129 [details]
Patch

[Kate Cheney]

Thanks Chris!

[Chris Dumez]

Comment on attachment 377129 [details]
Patch

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

r=me with fix.

> Source/WebCore/loader/ResourceLoadObserver.cpp:403
> +        iter.value->clear();

Rather than doing this, I would do m_perSessionResourceStatisticsMap.clear() *after* the loop. No reason to keep the sessionIDs in the map.

[Kate Cheney]

Created attachment 377133 [details]
Patch

[Chris Dumez]

Comment on attachment 377133 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Once you get a r+ with changes, you can already put the reviewer's name in the change logs so that the reviewer does not have to r+ again. Just so you know in the future, I will r+ again here.

[Kate Cheney]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 377133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377133&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> Once you get a r+ with changes, you can already put the reviewer's name in
> the change logs so that the reviewer does not have to r+ again. Just so you
> know in the future, I will r+ again here.

Thanks! I'll make sure to do this next time

[WebKit Commit Bot]

Comment on attachment 377133 [details]
Patch

Clearing flags on attachment: 377133

Committed r249056: <https://trac.webkit.org/changeset/249056>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.