Bug 215382 - Deferred WKUserScripts are exponentially injected on preloaded pages with frames
Summary: Deferred WKUserScripts are exponentially injected on preloaded pages with frames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-11 10:24 PDT by Timothy Hatcher
Modified: 2020-08-11 12:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.46 KB, patch)
2020-08-11 10:32 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2020-08-11 12:19 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2020-08-11 10:24:46 PDT
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>
Comment 1 Timothy Hatcher 2020-08-11 10:32:12 PDT
Created attachment 406390 [details]
Patch
Comment 2 Geoffrey Garen 2020-08-11 10:44:49 PDT
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, { }))
    ...
Comment 3 Sam Weinig 2020-08-11 10:46:32 PDT
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)
Comment 4 Sam Weinig 2020-08-11 10:47:35 PDT
Comment on attachment 406390 [details]
Patch

Eek, sorry, did not meant to change the review flag state.
Comment 5 Timothy Hatcher 2020-08-11 12:19:35 PDT
Created attachment 406402 [details]
Patch
Comment 6 Timothy Hatcher 2020-08-11 12:20:22 PDT
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.
Comment 7 EWS 2020-08-11 12:52:36 PDT
Committed r265510: <https://trac.webkit.org/changeset/265510>

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