Bug 202061 – SubFrameSOAuthorizationSession should ensure messages are posted in the right order to the parent frame

• Component: WebKit Misc.
• Status: NEW

[Jiewen Tan]

SubFrameSOAuthorizationSession should ensure messages are posted in order to the parent frame.

[Jiewen Tan]

<rdar://problem/55485666>

[Jiewen Tan]

Created attachment 379283 [details]
Patch

[youenn fablet]

Comment on attachment 379283 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:66
> +    if (auto* frame = page->process().webFrame(*m_frameID))

What guarantees that m_frameID is not null?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:73
> +    appendRequestToLoad(URL(navigationAction()->request().url()), IPC::DataReference(reinterpret_cast<const uint8_t*>(soAuthorizationPostDidCancelMessageToParent), strlen(soAuthorizationPostDidCancelMessageToParent)).vector());

Is there a more direct way to create a Vector<uint8_t> from a null terminated string?
If not, I would add a static routine to ease readability since this is also used below.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:74
> +    appendRequestToLoad(URL(navigationAction()->request().url()), String(navigationAction()->request().httpReferrer()));

Should soAuthorizationPostDidCancelMessageToParent do both the postMessage and the navigation (through window.location for instance)  or do we prefer to have them as two different tasks?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:100
> +    if (m_requestsToLoad.first().first != url)

I think this is ok but just in case, are we 100% sure m_requestsToLoad cannot be empty here?

> Source/WebKit/UIProcess/FrameLoadState.h:78
> +    HashMap<Observer*, WeakPtr<Observer>> m_observers;

Can we use a WeakHashSet?

[Jiewen Tan]

Comment on attachment 379283 [details]
Patch

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

Thanks Youenn for taking a look.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:66
>> +    if (auto* frame = page->process().webFrame(*m_frameID))
> 
> What guarantees that m_frameID is not null?

Right, thanks for pointing it out.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:73
>> +    appendRequestToLoad(URL(navigationAction()->request().url()), IPC::DataReference(reinterpret_cast<const uint8_t*>(soAuthorizationPostDidCancelMessageToParent), strlen(soAuthorizationPostDidCancelMessageToParent)).vector());
> 
> Is there a more direct way to create a Vector<uint8_t> from a null terminated string?
> If not, I would add a static routine to ease readability since this is also used below.

Sure, I will make a static helper that replicates what the method is doing. Maybe we should consider a Vector constructor that do the magic.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:74
>> +    appendRequestToLoad(URL(navigationAction()->request().url()), String(navigationAction()->request().httpReferrer()));
> 
> Should soAuthorizationPostDidCancelMessageToParent do both the postMessage and the navigation (through window.location for instance)  or do we prefer to have them as two different tasks?

Not sure if there is a way to preserve the http referrer in JS.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:100
>> +    if (m_requestsToLoad.first().first != url)
> 
> I think this is ok but just in case, are we 100% sure m_requestsToLoad cannot be empty here?

By design, yes. I will add an assertion.

> Source/WebKit/UIProcess/FrameLoadState.h:47
> +        virtual void didFinishLoad(const URL&) = 0;

Maybe we don't need to pass a URL?

>> Source/WebKit/UIProcess/FrameLoadState.h:78
>> +    HashMap<Observer*, WeakPtr<Observer>> m_observers;
> 
> Can we use a WeakHashSet?

I like new wtf helpers!!!

[Jiewen Tan]

Comment on attachment 379283 [details]
Patch

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

>>> Source/WebKit/UIProcess/FrameLoadState.h:78
>>> +    HashMap<Observer*, WeakPtr<Observer>> m_observers;
>> 
>> Can we use a WeakHashSet?
> 
> I like new wtf helpers!!!

It looks like WeakHashSet can't be copied. And nether does HashSet. Not sure why. And also not sure if I should make WeakHashSet copiable.

[Jiewen Tan]

Comment on attachment 379283 [details]
Patch

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

>>>> Source/WebKit/UIProcess/FrameLoadState.h:78
>>>> +    HashMap<Observer*, WeakPtr<Observer>> m_observers;
>>> 
>>> Can we use a WeakHashSet?
>> 
>> I like new wtf helpers!!!
> 
> It looks like WeakHashSet can't be copied. And nether does HashSet. Not sure why. And also not sure if I should make WeakHashSet copiable.

Filed Bug 202248 as a follow up.

[Jiewen Tan]

Created attachment 379587 [details]
Patch

[youenn fablet]

Comment on attachment 379587 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:65
> +    if (auto* targetFrame = this->navigationAction()->targetFrame()) {

Looking at SubFrameSOAuthorizationSession::create, I believe we always have a FrameID.
It might be simpler to pass it as a parameter and make m_frameID not Optional.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:98
> +    appendRequestToLoad(URL(response.url()), IPC::DataReference(reinterpret_cast<const uint8_t*>(data.bytes), data.length).vector());

Could use convertBytesToVector

> Source/WebKit/UIProcess/FrameLoadState.cpp:93
> +    auto observersCopy = m_observers;

A hashmap copy is probably more expensive than creating a Vector of values.
Maybe we could use a WeakHashSet for m_observers and a Vector<Observer*> for observersCopy.

> Source/WebKit/UIProcess/WebProcessPool.cpp:44
> +#include "LegacyGlobalSettings.h"

Is it needed?

[Jiewen Tan]

Comment on attachment 379587 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:65
>> +    if (auto* targetFrame = this->navigationAction()->targetFrame()) {
> 
> Looking at SubFrameSOAuthorizationSession::create, I believe we always have a FrameID.
> It might be simpler to pass it as a parameter and make m_frameID not Optional.

Sounds good. Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:98
>> +    appendRequestToLoad(URL(response.url()), IPC::DataReference(reinterpret_cast<const uint8_t*>(data.bytes), data.length).vector());
> 
> Could use convertBytesToVector

Good catch!

>> Source/WebKit/UIProcess/FrameLoadState.cpp:93
>> +    auto observersCopy = m_observers;
> 
> A hashmap copy is probably more expensive than creating a Vector of values.
> Maybe we could use a WeakHashSet for m_observers and a Vector<Observer*> for observersCopy.

Fixed. A good idea!

>> Source/WebKit/UIProcess/WebProcessPool.cpp:44
>> +#include "LegacyGlobalSettings.h"
> 
> Is it needed?

I made some arrangement of Sources.txt during the development and discover this lack of header. Just did it for someone else's convenience.

[Jiewen Tan]

Created attachment 379628 [details]
Patch

[youenn fablet]

Comment on attachment 379628 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:-107
> -    if (!page || !navigationActionPtr)

We were doing a page check here and no longer now.
Might be safer to keep it.

> Source/WebKit/UIProcess/FrameLoadState.cpp:95
> +        observersCopy.append(&observer);

We can use uncheckedAppend/resverInitialCapacity.
Or better, just use copyToVector if working or WTF::map(m_observers, [](auto& observer) { return observer.get(); });

[Jiewen Tan]

Comment on attachment 379628 [details]
Patch

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

Thanks Youenn for r+ the patch.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:-107
>> -    if (!page || !navigationActionPtr)
> 
> We were doing a page check here and no longer now.
> Might be safer to keep it.

Sure.

>> Source/WebKit/UIProcess/FrameLoadState.cpp:95
>> +        observersCopy.append(&observer);
> 
> We can use uncheckedAppend/resverInitialCapacity.
> Or better, just use copyToVector if working or WTF::map(m_observers, [](auto& observer) { return observer.get(); });

I was thinking about Vector.appendRange but then it doesn't work. It definitely looks much better!

[Jiewen Tan]

Comment on attachment 379628 [details]
Patch

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

>>> Source/WebKit/UIProcess/FrameLoadState.cpp:95
>>> +        observersCopy.append(&observer);
>> 
>> We can use uncheckedAppend/resverInitialCapacity.
>> Or better, just use copyToVector if working or WTF::map(m_observers, [](auto& observer) { return observer.get(); });
> 
> I was thinking about Vector.appendRange but then it doesn't work. It definitely looks much better!

WeakHashSet uses computeSize() instead of size(), and therefore WTF::map doesn't work.

[Jiewen Tan]

Created attachment 379713 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 379713 [details]
Patch for landing

Clearing flags on attachment: 379713

Committed r250416: <https://trac.webkit.org/changeset/250416>