Bug 176941 – Storage Access API: Web process should ask UI process for grant/deny

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

The web process needs to asynchronously ask the UI process whether to grant or deny storage access.

[Radar WebKit Bug Importer]

<rdar://problem/34440036>

[John Wilander]

Created attachment 322625 [details]
Patch

[John Wilander]

Style check errors are the callback function parameters.

[Build Bot]

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.

[Sam Weinig]

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.

[John Wilander]

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!

[John Wilander]

Created attachment 322724 [details]
Patch

[Build Bot]

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.

[Chris Dumez]

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?

[John Wilander]

Created attachment 322872 [details]
Patch for landing

[John Wilander]

Thanks, Chris! Fixed all your comments.

[WebKit Commit Bot]

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

[John Wilander]

Created attachment 322904 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 322904 [details]
Patch for landing

Clearing flags on attachment: 322904

Committed r222941: <http://trac.webkit.org/changeset/222941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.