Bug 229731 - Use after move in ServiceWorkerThreadProxy
Summary: Use after move in ServiceWorkerThreadProxy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-31 13:38 PDT by Kate Cheney
Modified: 2021-08-31 16:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.53 KB, patch)
2021-08-31 13:39 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (4.97 KB, patch)
2021-08-31 14:23 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-08-31 13:38:04 PDT
Use after move in ServiceWorkerThreadProxy
Comment 1 Kate Cheney 2021-08-31 13:39:03 PDT
Created attachment 436936 [details]
Patch
Comment 2 Chris Dumez 2021-08-31 13:40:25 PDT
Comment on attachment 436936 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:107
> +    std::optional<LastNavigationWasAppInitiated> m_lastNavigationWasAppInitiated;

Seems very unfortunate to have to add a new data member for something that is only used in the constructor.
Comment 3 Chris Dumez 2021-08-31 13:46:14 PDT
Comment on attachment 436936 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:122
> +        setLastNavigationWasAppInitiated(m_lastNavigationWasAppInitiated == LastNavigationWasAppInitiated::Yes);

Maybe the call site (WebSWContextManagerConnection::installServiceWorker()), could store data.lastNavigationWasAppInitiated before calling ServiceWorkerThreadProxy::create() and then call setLastNavigationWasAppInitiated() on the ServiceWorkerThreadProxy object?
Comment 4 Darin Adler 2021-08-31 13:47:19 PDT
Comment on attachment 436936 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:107
> +    std::optional<LastNavigationWasAppInitiated> m_lastNavigationWasAppInitiated;

Not good to store this value for the lifetime of the object just to convey its value from the start to the end of the constructor function.

Another model would be to use a lambda for this purpose. Something ike this:

    , m_document([this, &data] {
        auto& document = *m_page->mainFrame().document();
        if (data.lastNavigationWasAppInitiated) {
            if (auto loader = document.loader())
                loader->setLastNavigationWasAppInitiated(data.lastNavigationWasAppInitiated == LastNavigationWasAppInitiated::Yes);
        }
        return document;
    })

Or could use a named function.
Comment 5 Kate Cheney 2021-08-31 14:23:36 PDT
Created attachment 436946 [details]
Patch
Comment 6 Kate Cheney 2021-08-31 14:26:11 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 436936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436936&action=review
> 
> > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:107
> > +    std::optional<LastNavigationWasAppInitiated> m_lastNavigationWasAppInitiated;
> 
> Not good to store this value for the lifetime of the object just to convey
> its value from the start to the end of the constructor function.
> 
> Another model would be to use a lambda for this purpose. Something ike this:
> 
>     , m_document([this, &data] {
>         auto& document = *m_page->mainFrame().document();
>         if (data.lastNavigationWasAppInitiated) {
>             if (auto loader = document.loader())
>                
> loader->setLastNavigationWasAppInitiated(data.lastNavigationWasAppInitiated
> == LastNavigationWasAppInitiated::Yes);
>         }
>         return document;
>     })
> 
> Or could use a named function.

I ended up going with Chris's suggestion because it avoids some code duplication by using ServiceWorkerThreadProxy::setLastNavigationWasAppInitiated().
Comment 7 EWS 2021-08-31 16:11:01 PDT
Committed r281831 (241164@main): <https://commits.webkit.org/241164@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436946 [details].
Comment 8 Radar WebKit Bug Importer 2021-08-31 16:12:24 PDT
<rdar://problem/82601336>