Bug 202608 – DOMCacheStorage should not prevent pages from entering the back/forward cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

DOMCacheStorage should not prevent pages from entering the back/forward cache.

[Chris Dumez]

Created attachment 380261 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56036497>

[Chris Dumez]

Created attachment 380355 [details]
Patch

[Chris Dumez]

Comment on attachment 380355 [details]
Patch

Ready for review.

[Chris Dumez]

ping review?

[youenn fablet]

Comment on attachment 380355 [details]
Patch

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

> Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223
> +            });

Shouldn’t we create the cache at the time we execute the enquêtes task?

[Chris Dumez]

Comment on attachment 380355 [details]
Patch

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

>> Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223
>> +            });
> 
> Shouldn’t we create the cache at the time we execute the enquêtes task?

enquêtes huh? :) How French.

I actually wasn't sure about the right time to append the cache to m_caches (assuming this is what you meant). I could do it in the lambda, or outside like I ended up doing.
It looks to me like the current code is unsafe when there are several parallel requests to doOpen() for the same name. You may end up creating the 2 DOMCache objects for the same name and have 2 caches for the same name in m_caches. Is that right or am I missing something?
The reason I appended the cache to m_caches synchronously rather than in the enqueued task was to not make this *race* worse. Also, while I see a good reason to resolve the promise in an async task, I did not see a need to delay the creation of the cache or adding it to m_caches.

[youenn fablet]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 380355 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380355&action=review
> 
> >> Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223
> >> +            });
> > 
> > Shouldn’t we create the cache at the time we execute the enquêtes task?
> 
> enquêtes huh? :) How French.

I did the enquete and the culprit....my phone autocompletion
enqueued -> enquêtes!

On the other side, enquete -> enquire...

> I actually wasn't sure about the right time to append the cache to m_caches
> (assuming this is what you meant). I could do it in the lambda, or outside
> like I ended up doing.
> It looks to me like the current code is unsafe when there are several
> parallel requests to doOpen() for the same name. You may end up creating the
> 2 DOMCache objects for the same name and have 2 caches for the same name in
> m_caches. Is that right or am I missing something?
> The reason I appended the cache to m_caches synchronously rather than in the
> enqueued task was to not make this *race* worse. Also, while I see a good
> reason to resolve the promise in an async task, I did not see a need to
> delay the creation of the cache or adding it to m_caches.

Well, we could be suspended and DOMCache is an ActiveDOMObject.
Also, creating an object while we might never use it is not great.

I guess the alternative would be to not make DOMCache an ActiveDOMObject and instead have the CacheStorageConnection be an ActiveDOMObject (hence being owned by the Document). The CacheStorageConnection would then buffer all the callbacks if needed.
I guess we can do that if reducing the number of ActiveDOMObjects is motivating enough.

As of the race, if you call open with the same name, you will end up with two JSDOMCache pointing to the same request/response set.
I believe this is as per spec and the creation of the cache object is synchronous with the promise resolution.

We are doing some caching in DOMCacheStorage for skipping sending some IPC to network process. It should not (at least in theory) change the observable JS behaviour, except for getting faster promise resolutions.

[Chris Dumez]

(In reply to youenn fablet from comment #8)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 380355 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380355&action=review
> > 
> > >> Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223
> > >> +            });
> > > 
> > > Shouldn’t we create the cache at the time we execute the enquêtes task?
> > 
> > enquêtes huh? :) How French.
> 
> I did the enquete and the culprit....my phone autocompletion
> enqueued -> enquêtes!
> 
> On the other side, enquete -> enquire...
> 
> > I actually wasn't sure about the right time to append the cache to m_caches
> > (assuming this is what you meant). I could do it in the lambda, or outside
> > like I ended up doing.
> > It looks to me like the current code is unsafe when there are several
> > parallel requests to doOpen() for the same name. You may end up creating the
> > 2 DOMCache objects for the same name and have 2 caches for the same name in
> > m_caches. Is that right or am I missing something?
> > The reason I appended the cache to m_caches synchronously rather than in the
> > enqueued task was to not make this *race* worse. Also, while I see a good
> > reason to resolve the promise in an async task, I did not see a need to
> > delay the creation of the cache or adding it to m_caches.
> 
> Well, we could be suspended and DOMCache is an ActiveDOMObject.

I do not think this is an issue. AFAIK, it is perfectly fine to create ActiveDOMObjects while suspended, this is exactly why you must call suspendIfNeeded() on an ActiveDOMObject right after constructing. What's not allowed is constructing new ActiveDOMObject in the implementation of suspend() / resume().

> Also, creating an object while we might never use it is not great.

Fair point.

> I guess the alternative would be to not make DOMCache an ActiveDOMObject and
> instead have the CacheStorageConnection be an ActiveDOMObject (hence being
> owned by the Document). The CacheStorageConnection would then buffer all the
> callbacks if needed.
> I guess we can do that if reducing the number of ActiveDOMObjects is
> motivating enough.
> 
> As of the race, if you call open with the same name, you will end up with
> two JSDOMCache pointing to the same request/response set.
> I believe this is as per spec and the creation of the cache object is
> synchronous with the promise resolution.

Ok, as long as you're not concerned about the potential race, then I'll move the cache creation.

[Chris Dumez]

Created attachment 380554 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380554 [details]
Patch

Clearing flags on attachment: 380554

Committed r250965: <https://trac.webkit.org/changeset/250965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.