Bug 202608

Created attachment 380261 [details] Patch

<rdar://problem/56036497>

Created attachment 380355 [details] Patch

Comment on attachment 380355 [details] Patch Ready for review.

ping review?

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?

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.

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

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

Created attachment 380554 [details] Patch

Comment on attachment 380554 [details] Patch Clearing flags on attachment: 380554 Committed r250965: <https://trac.webkit.org/changeset/250965>

All reviewed patches have been landed. Closing bug.