Bug 215382 – Deferred WKUserScripts are exponentially injected on preloaded pages with frames

• Component: WebKit API
• Status: RESOLVED FIXED

[Timothy Hatcher]

We inject a web extension script multiple times per frame if the page is preloaded in Safari and has multiple frames. The number of times we inject is how many frames the page has. So this is exponential.

<rdar://problem/66837802>

[Timothy Hatcher]

Created attachment 406390 [details]
Patch

[Geoffrey Garen]

Comment on attachment 406390 [details]
Patch

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

r=me

> Source/WebCore/page/Frame.cpp:694
> +void Frame::injectUserScriptsAwaitingNotification()
> +{
> +    for (const auto& pair : m_userScriptsAwaitingNotification)
> +        injectUserScriptImmediately(pair.first, pair.second.get());
> +
> +    m_userScriptsAwaitingNotification.clear();
> +}

I prefer the idiom where you take() or WTFMove() or exchange() before entering the loop. This means the data member is always coherent, even during the loop. One reason this might matter is re-entrancy. But even if we think re-entrancy won't happen, I think the idiom is a bit clearer.

In this case, you even get to delete a line of code:

for (const auto& pair : std::exchange(m_userScriptsAwaitingNotification, { }))
    ...

[Sam Weinig]

Comment on attachment 406390 [details]
Patch

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

> Source/WebCore/page/Frame.cpp:690
> +    for (const auto& pair : m_userScriptsAwaitingNotification)

You can use structured bindings to make this a bit nicer:

for (const auto& [world, script] : m_userScriptsAwaitingNotification)

[Sam Weinig]

Comment on attachment 406390 [details]
Patch

Eek, sorry, did not meant to change the review flag state.

[Timothy Hatcher]

Created attachment 406402 [details]
Patch

[Timothy Hatcher]

Comment on attachment 406390 [details]
Patch

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

>> Source/WebCore/page/Frame.cpp:690
>> +    for (const auto& pair : m_userScriptsAwaitingNotification)
> 
> You can use structured bindings to make this a bit nicer:
> 
> for (const auto& [world, script] : m_userScriptsAwaitingNotification)

Cool. Did this.

>> Source/WebCore/page/Frame.cpp:694
>> +}
> 
> I prefer the idiom where you take() or WTFMove() or exchange() before entering the loop. This means the data member is always coherent, even during the loop. One reason this might matter is re-entrancy. But even if we think re-entrancy won't happen, I think the idiom is a bit clearer.
> 
> In this case, you even get to delete a line of code:
> 
> for (const auto& pair : std::exchange(m_userScriptsAwaitingNotification, { }))
>     ...

Done.

[EWS]

Committed r265510: <https://trac.webkit.org/changeset/265510>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406402 [details].