Use after move in ServiceWorkerThreadProxy
Created attachment 436936 [details] Patch
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 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 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.
Created attachment 436946 [details] Patch
(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().
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].
<rdar://problem/82601336>