Bug 202012

<rdar://problem/55285295>

Created attachment 379171 [details] Patch

Created attachment 379172 [details] Patch

Created attachment 379173 [details] Patch

Comment on attachment 379173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379173&action=review Thank you for jumping on this bug. r=me > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:-225 > - } Is this the Mail case? > Source/WebKit/UIProcess/WebPageProxy.cpp:1186 > + if (!process->isLaunching() || !url.isLocalFile()) Why don't we call into the WebPageProxy::loadFile code path when the url is a local file? Are there cases that are "isLocalFile" but shouldn't use the file loading path? > Source/WebKit/UIProcess/WebPageProxy.cpp:1241 > + if (!m_process->isLaunching()) This would be clearer if we flipped the check: if (m_process->isLaunching()) ... Send the 'LoadRequestWaitingForPID' message else .... Send the 'LoadRequest' message. But I guess that is the reverse of the "load from network' case.

Comment on attachment 379173 [details] Patch r=me Can we regression test this? (Maybe possible through an API test that issues a file URL load?) > Source/WebKit/ChangeLog:10 > + the same logic to initializing the sandbox extension if the process has already to initialize had already > Source/WebKit/ChangeLog:13 > + the client did not provided one. The logic in maybeInitializeSandboxExtensionHandle() provide

(In reply to Geoffrey Garen from comment #6) > Comment on attachment 379173 [details] > Patch > > r=me > > Can we regression test this? (Maybe possible through an API test that issues > a file URL load?) No because our API tests are not sandboxed. > > > Source/WebKit/ChangeLog:10 > > + the same logic to initializing the sandbox extension if the process has already > > to initialize > > had already > > > Source/WebKit/ChangeLog:13 > > + the client did not provided one. The logic in maybeInitializeSandboxExtensionHandle() > > provide

Comment on attachment 379173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379173&action=review >> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:-225 >> - } > > Is this the Mail case? The mail case is https://trac.webkit.org/changeset/247400/webkit, which is in maybeInitializeSandboxExtensionHandle(). This is why I am now calling maybeInitializeSandboxExtensionHandle(). >> Source/WebKit/UIProcess/WebPageProxy.cpp:1186 >> + if (!process->isLaunching() || !url.isLocalFile()) > > Why don't we call into the WebPageProxy::loadFile code path when the url is a local file? Are there cases that are "isLocalFile" but shouldn't use the file loading path? I would need to look into it more to see if there would be side effects from calling loadFile(). In any case, I'd rather not do this in this patch to reduce risk. >> Source/WebKit/UIProcess/WebPageProxy.cpp:1241 >> + if (!m_process->isLaunching()) > > This would be clearer if we flipped the check: > > if (m_process->isLaunching()) > ... Send the 'LoadRequestWaitingForPID' message > else > .... Send the 'LoadRequest' message. > > But I guess that is the reverse of the "load from network' case. Ok.

Created attachment 379174 [details] Patch

Comment on attachment 379174 [details] Patch Clearing flags on attachment: 379174 Committed r250110: <https://trac.webkit.org/changeset/250110>

All reviewed patches have been landed. Closing bug.

Comment on attachment 379174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379174&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:1239 > > m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL); I think this line should be removed, since this is handled inside maybeInitializeSandboxExtensionHandle now, and can cause a later call of maybeInitializeSandboxExtensionHandle to just bail if read access is already assumed.

(In reply to Per Arne Vollan from comment #12) > Comment on attachment 379174 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379174&action=review > > > Source/WebKit/UIProcess/WebPageProxy.cpp:1239 > > > > m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL); > > I think this line should be removed, since this is handled inside > maybeInitializeSandboxExtensionHandle now, and can cause a later call of > maybeInitializeSandboxExtensionHandle to just bail if read access is already > assumed. Good catch, thanks.

Reopen to fix issue found by Per Arne.

Created attachment 379238 [details] Patch

Comment on attachment 379238 [details] Patch Thanks for fixing this bug! R=me.

Comment on attachment 379238 [details] Patch Clearing flags on attachment: 379238 Committed r250130: <https://trac.webkit.org/changeset/250130>

All reviewed patches have been landed. Closing bug.