Bug 229731 – Use after move in ServiceWorkerThreadProxy

• Component: New Bugs
• Status: RESOLVED FIXED

[Kate Cheney]

Use after move in ServiceWorkerThreadProxy

[Kate Cheney]

Created attachment 436936 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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?

[Darin Adler]

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.

[Kate Cheney]

Created attachment 436946 [details]
Patch

[Kate Cheney]

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

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/82601336>