Bug 203772 - Port DOMCache / DOMCacheStorage to the HTML5 event loop
Summary: Port DOMCache / DOMCacheStorage to the HTML5 event loop
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:
Blocks: 202843
  Show dependency treegraph
 
Reported: 2019-11-01 17:03 PDT by Chris Dumez
Modified: 2019-11-02 20:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.40 KB, patch)
2019-11-01 17:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.01 KB, patch)
2019-11-02 12:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.03 KB, patch)
2019-11-02 19:59 PDT, 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 2019-11-01 17:03:18 PDT
Port DOMCache / DOMCacheStorage to the HTML5 event loop.
Comment 1 Chris Dumez 2019-11-01 17:04:09 PDT
Created attachment 382656 [details]
Patch
Comment 2 Chris Dumez 2019-11-02 12:03:57 PDT
I suspect a bug in the WorkerEventLoop somehow. The cache-storage.https.html is passing for window but failing for workers.
Comment 3 Chris Dumez 2019-11-02 12:29:50 PDT
Created attachment 382683 [details]
Patch
Comment 4 Chris Dumez 2019-11-02 12:30:22 PDT
Comment on attachment 382683 [details]
Patch

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

> Source/WebCore/workers/WorkerEventLoop.cpp:-101
> -    for (auto& task : m_tasks)

Found the WorkerEventLoop bug.
Comment 5 Sam Weinig 2019-11-02 19:43:39 PDT
Comment on attachment 382683 [details]
Patch

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

> Source/WebCore/workers/WorkerEventLoop.cpp:101
> +    auto tasks = WTFMove(m_tasks);

I think the kosher way of doing this is to swap with an empty Vector<Task>. Otherwise, it leaves m_tasks in a bad (though currently kind of ok) state.
Comment 6 Chris Dumez 2019-11-02 19:49:58 PDT
(In reply to Sam Weinig from comment #5)
> Comment on attachment 382683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382683&action=review
> 
> > Source/WebCore/workers/WorkerEventLoop.cpp:101
> > +    auto tasks = WTFMove(m_tasks);
> 
> I think the kosher way of doing this is to swap with an empty Vector<Task>.
> Otherwise, it leaves m_tasks in a bad (though currently kind of ok) state.

We guarantee that our WebKit types are in OK state after moving. We even have our own Optional because of this. I don’t really mind swapping with an empty vector but it is less concise and not necessary with WebKit types. We use this pattern a lot in WebKit too, although I know not everybody likes it.
Comment 7 Chris Dumez 2019-11-02 19:59:09 PDT
Created attachment 382687 [details]
Patch
Comment 8 WebKit Commit Bot 2019-11-02 20:44:36 PDT
Comment on attachment 382687 [details]
Patch

Clearing flags on attachment: 382687

Committed r251968: <https://trac.webkit.org/changeset/251968>
Comment 9 WebKit Commit Bot 2019-11-02 20:44:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-11-02 20:45:20 PDT
<rdar://problem/56844994>