Bug 236985 - Avoid having to iterate the whole frame tree(s) every time we need to dispatch storage events
Summary: Avoid having to iterate the whole frame tree(s) every time we need to dispatc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 236871
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-21 10:56 PST by Chris Dumez
Modified: 2022-02-22 08:10 PST (History)
11 users (show)

See Also:


Attachments
Patch (37.13 KB, patch)
2022-02-21 11:03 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.08 KB, patch)
2022-02-21 20:11 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2022-02-21 10:56:56 PST
Avoid having to iterate the whole frame tree(s) every time we need to dispatch storage events.
Comment 1 Chris Dumez 2022-02-21 11:03:44 PST
Created attachment 452748 [details]
Patch
Comment 2 Darin Adler 2022-02-21 18:14:54 PST
Comment on attachment 452748 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:168
> +    for (auto& window : windowsInterestedInStorageEvents())
> +        apply(window);

Maybe the use of Vector<Ref<DOMWindow> should be inside this function rather than in the functions that call it. I suppose that’s less efficient since we will create an extra vector, but it seems dangerous to have a function that’s so easy to use unsafely.

> Source/WebCore/page/DOMWindow.cpp:458
> +    windowsInterestedInStorageEvents().remove(*this);

How important is it to do this explicitly in a WeakHashSet? If we have this code, it seems to make it safe to use HashSet instead.

> Source/WebCore/storage/StorageEventDispatcher.cpp:43
> +static void dispatchSessionStorageEventsToWindows(Page& page, const Vector<Ref<DOMWindow>>& windows, const String& key, const String& oldValue, const String& newValue, const String& url, const SecurityOrigin& securityOrigin)

Can we factor these two functions so they share almost all the code? There are only two differences here, StorageType and what function we use to get the storage object.
Comment 3 Chris Dumez 2022-02-21 18:21:43 PST
Comment on attachment 452748 [details]
Patch

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

>> Source/WebCore/page/DOMWindow.cpp:168
>> +        apply(window);
> 
> Maybe the use of Vector<Ref<DOMWindow> should be inside this function rather than in the functions that call it. I suppose that’s less efficient since we will create an extra vector, but it seems dangerous to have a function that’s so easy to use unsafely.

So a copyToVector() here and loop over that to protect against the apply function modifying the container?

>> Source/WebCore/page/DOMWindow.cpp:458
>> +    windowsInterestedInStorageEvents().remove(*this);
> 
> How important is it to do this explicitly in a WeakHashSet? If we have this code, it seems to make it safe to use HashSet instead.

We are supposed to remove entries from WeakHashSet as far as I know, in order to recover capacity (shrink only happens for some operations). Could totally use a HashSet of raw pointers but it would make Ryosuke angry :) I was told we should avoid containers of raw pointers due to security bugs. So we use WeakHashSet and use it like a regular HashSet.
Comment 4 Darin Adler 2022-02-21 18:25:48 PST
Comment on attachment 452748 [details]
Patch

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

>>> Source/WebCore/page/DOMWindow.cpp:168
>>> +        apply(window);
>> 
>> Maybe the use of Vector<Ref<DOMWindow> should be inside this function rather than in the functions that call it. I suppose that’s less efficient since we will create an extra vector, but it seems dangerous to have a function that’s so easy to use unsafely.
> 
> So a copyToVector() here and loop over that to protect against the apply function modifying the container?

Yes. It’s wasteful given how we are currently using this, but safer. As you probably already know, we do that in Page::forEachDocument, for example.
Comment 5 Chris Dumez 2022-02-21 20:11:51 PST
Created attachment 452819 [details]
Patch
Comment 6 EWS 2022-02-22 01:49:01 PST
Committed r290295 (247618@main): <https://commits.webkit.org/247618@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452819 [details].
Comment 7 Radar WebKit Bug Importer 2022-02-22 01:50:18 PST
<rdar://problem/89282659>
Comment 8 Chris Dumez 2022-02-22 07:15:12 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 452748 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452748&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:168
> > +    for (auto& window : windowsInterestedInStorageEvents())
> > +        apply(window);
> 
> Maybe the use of Vector<Ref<DOMWindow> should be inside this function rather
> than in the functions that call it. I suppose that’s less efficient since we
> will create an extra vector, but it seems dangerous to have a function
> that’s so easy to use unsafely.
> 
> > Source/WebCore/page/DOMWindow.cpp:458
> > +    windowsInterestedInStorageEvents().remove(*this);
> 
> How important is it to do this explicitly in a WeakHashSet? If we have this
> code, it seems to make it safe to use HashSet instead.
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:43
> > +static void dispatchSessionStorageEventsToWindows(Page& page, const Vector<Ref<DOMWindow>>& windows, const String& key, const String& oldValue, const String& newValue, const String& url, const SecurityOrigin& securityOrigin)
> 
> Can we factor these two functions so they share almost all the code? There
> are only two differences here, StorageType and what function we use to get
> the storage object.

Oh, I missed this comment. I'll look into it today.