Bug 236985 – Avoid having to iterate the whole frame tree(s) every time we need to dispatch storage events

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Avoid having to iterate the whole frame tree(s) every time we need to dispatch storage events.

[Chris Dumez]

Created attachment 452748 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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.

[Darin Adler]

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.

[Chris Dumez]

Created attachment 452819 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/89282659>

[Chris Dumez]

(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.