1348874 - Timeout navigator.storage.persist() in sandboxed iframe should reject with TypeError

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This test case just recently added.

Timeout	navigator.storage.persist() in sandboxed iframe should reject with TypeError	Test timed out

Not Run	navigator.storage.persisted() in non-sandboxed iframe should not reject	
Not Run	navigator.storage.persisted() in sandboxed iframe should reject with TypeError	
Not Run	navigator.storage.estimate() in non-sandboxed iframe should not reject	
Not Run	navigator.storage.estimate() in sandboxed iframe should reject with TypeError

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

We have this issue just because storage standard has been updated last month.


https://github.com/whatwg/storage/commit/0dbd146e774316d44182051faf21654ef23ed7cf

Disable the API in opaque origins
Also consistently queue a task to reject and resolve promises from in parallel steps.
Tests: w3c/web-platform-tests#4919.

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1348874 - If origin is an opaque origin, reject promise with a TypeError

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8850529 [details] [diff] [review]
Bug 1348874 - If origin is an opaque origin, reject promise with a TypeError

The specification updated 28 days ago.
And the new wpt test case has been imported into our codebase.
This patch fixes opaque-origin.https.html test case.

In addition, prefs are enabled for automatic test purpose, otherwise this test case will be blocked by permission request dialog.

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1348874 - If origin is an opaque origin, reject promise with a TypeError, r=janv

Rebase again since Bug 1286717 Part 1 got r+.

Comment 5

[Jan Varga [:janv]]

Comment on attachment 8856504 [details] [diff] [review]
Bug 1348874 - If origin is an opaque origin, reject promise with a TypeError, r=janv

Review of attachment 8856504 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/StorageManager.cpp
@@ +289,5 @@
>  
> +    // Storage Standard 7. API
> +    // If origin is an opaque origin, then reject promise with a TypeError.
> +    if (principal->GetIsNullPrincipal()) {
> +      promise->MaybeReject(NS_ERROR_DOM_TYPE_ERR);

Isn't this the same problem as with private browsing ?
I'm not sure, but I think we shouldn't call MaybeResolve/Reject() before we actually return the promise to the caller.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Jan Varga [:janv] from comment #5)
> Comment on attachment 8856504 [details] [diff] [review]
> Bug 1348874 - If origin is an opaque origin, reject promise with a
> TypeError, r=janv
> 
> Review of attachment 8856504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/StorageManager.cpp
> @@ +289,5 @@
> >  
> > +    // Storage Standard 7. API
> > +    // If origin is an opaque origin, then reject promise with a TypeError.
> > +    if (principal->GetIsNullPrincipal()) {
> > +      promise->MaybeReject(NS_ERROR_DOM_TYPE_ERR);
> 
> Isn't this the same problem as with private browsing ?
> I'm not sure, but I think we shouldn't call MaybeResolve/Reject() before we
> actually return the promise to the caller.
I think MaybeReject() call will send to microtask queue. So this won't be executed immediately.
There are many places did the same thing. 

http://searchfox.org/mozilla-central/source/dom/file/FileCreatorHelper.cpp#60

http://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#6058

http://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeySession.cpp#357

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I will study this promise question a bit more and update to this bug.

Comment 8

[Jan Varga [:janv]]

Ok, never mind then.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Some notes for reject promise.

http://searchfox.org/mozilla-central/source/js/src/jsapi.cpp#4986
return PromiseObject::reject(cx, promise, rejectionValue);

http://searchfox.org/mozilla-central/source/js/src/builtin/Promise.cpp#3023
return RejectMaybeWrappedPromise(cx, promise, rejectionValue);

http://searchfox.org/mozilla-central/source/js/src/builtin/Promise.cpp#784
return ResolvePromise(cx, promise, reason, JS::PromiseState::Rejected);

http://searchfox.org/mozilla-central/source/js/src/builtin/Promise.cpp#627
return TriggerPromiseReactions(cx, reactionsVal, state, valueOrReason);

http://searchfox.org/mozilla-central/source/js/src/builtin/Promise.cpp#826
return EnqueuePromiseReactionJob(cx, reactions, valueOrReason, state);


http://searchfox.org/mozilla-central/source/js/src/vm/Runtime.cpp#711
return cx->runtime()->enqueuePromiseJobCallback(cx, job, allocationSite, incumbentGlobal, data);


http://searchfox.org/mozilla-central/source/xpcom/base/CycleCollectedJSContext.cpp#1059
self->DispatchToMicroTask(runnable.forget());

After calling DispatchToMicroTask(), this is async I think.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2f6f1f7bd6
If origin is an opaque origin, reject promise with a TypeError, r=janv

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This patch should be landed together with Bug 1286717.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/aa2f6f1f7bd6