The web process needs to asynchronously ask the UI process whether to grant or deny storage access.
<rdar://problem/34440036>
Created attachment 322625 [details] Patch
Style check errors are the callback function parameters.
Attachment 322625 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1370: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:247: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:345: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:6044: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.h:997: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.h:1595: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1226: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 9 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 322625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322625&action=review > Source/WebCore/dom/Document.cpp:7340 > StringBuilder builder; > builder.appendLiteral("Do you want to use your "); > - builder.append(partitionDomain); > + builder.append(iframeHost); > builder.appendLiteral(" ID on "); > - builder.append(topPartitionDomain); > + builder.append(topHost); > builder.appendLiteral("?"); This string is non-localized and presents non sanitized (via userVisibleHost) hosts. I also think this should be done by either WebKit or the hosting app as we do with other permission prompts. > Source/WebCore/dom/Document.cpp:7345 > if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) { > - m_hasStorageAccess = true; > - ResourceLoadObserver::shared().registerStorageAccess(partitionDomain, topPartitionDomain); > - promise->resolve<IDLBoolean>(true); > + uint64_t contextId = nextRequestStorageAccessContextId(); > + page->chrome().client().requestStorageAccess(WTFMove(iframeHost), WTFMove(topHost), contextId, [this, promise] (bool granted) { Why is the sync prompt still necessary? Can the prompting be handled by the requestStorageAccess now? It doesn't seem like this layer needs to generate the contextId since it only passes it in. Seems like something the WebKit layer could do.
Thanks for your comments, Sam! (In reply to Sam Weinig from comment #5) > Comment on attachment 322625 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322625&action=review > > > Source/WebCore/dom/Document.cpp:7340 > > StringBuilder builder; > > builder.appendLiteral("Do you want to use your "); > > - builder.append(partitionDomain); > > + builder.append(iframeHost); > > builder.appendLiteral(" ID on "); > > - builder.append(topPartitionDomain); > > + builder.append(topHost); > > builder.appendLiteral("?"); > > This string is non-localized and presents non sanitized (via > userVisibleHost) hosts. I also think this should be done by either WebKit > or the hosting app as we do with other permission prompts. As the FIXME indicates, this is just a placeholder. I might keep it there for demo purposes but in any kind of final version we will a) maybe not prompt at all, b) not use JavaScript confirm, c) localize any prompt, and d) only use TLD+1 in the prompt to communicate to the user what ID he/she is about to use. > > Source/WebCore/dom/Document.cpp:7345 > > if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) { > > - m_hasStorageAccess = true; > > - ResourceLoadObserver::shared().registerStorageAccess(partitionDomain, topPartitionDomain); > > - promise->resolve<IDLBoolean>(true); > > + uint64_t contextId = nextRequestStorageAccessContextId(); > > + page->chrome().client().requestStorageAccess(WTFMove(iframeHost), WTFMove(topHost), contextId, [this, promise] (bool granted) { > > Why is the sync prompt still necessary? Can the prompting be handled by the > requestStorageAccess now? See explanation above. > It doesn't seem like this layer needs to generate the contextId since it > only passes it in. Seems like something the WebKit layer could do. I'll see if I can move it up to WebKit. Thanks!
Created attachment 322724 [details] Patch
Attachment 322724 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1370: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:247: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:345: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:6050: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.h:997: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.h:1595: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1226: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 9 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 322724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322724&action=review r=me with comments > Source/WebCore/dom/Document.cpp:7338 > + page->chrome().client().requestStorageAccess(WTFMove(iframeHost), WTFMove(topHost), [this, promise] (bool granted) { Should the lambda capture a weakPtr to the Document? What guarantees the document is still alive when the lambda is called asynchronously? > Source/WebCore/page/ChromeClient.h:467 > + virtual void requestStorageAccess(String&& /*subFrameHost*/, String&& /*topFrameHost*/, WTF::Function<void (bool)>&& /*callback*/) { } The default implementation should call the callback to avoid hangs. > Source/WebKit/UIProcess/WebPageProxy.cpp:7045 > + m_websiteDataStore->requestStorageAccess(WTFMove(subFrameHost), WTFMove(topFrameHost), [this, webProcessContextId] (bool granted) { We need a is/was/has prefix for boolean parameters as per coding style. I would rename granted to wasGranted everywhere. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1373 > + return; You need to call the callback before returning to avoid hangs. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6053 > + m_storageAccessResponseCallbackMap.set(contextId, WTFMove(callback)); Should probably be a add(), not a set(). Should can also assert that addResult.isNewEntry to be safe. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6060 > + if (callback) Shouldn't this be an assertion?
Created attachment 322872 [details] Patch for landing
Thanks, Chris! Fixed all your comments.
Comment on attachment 322872 [details] Patch for landing Rejecting attachment 322872 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ntextPrivateMac.dia -c /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/API/C/mac/WKContextPrivateMac.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WKContextPrivateMac.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WebPage.o WebProcess/WebPage/WebPage.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/4770472
Created attachment 322904 [details] Patch for landing
Comment on attachment 322904 [details] Patch for landing Clearing flags on attachment: 322904 Committed r222941: <http://trac.webkit.org/changeset/222941>
All reviewed patches have been landed. Closing bug.