Closed Bug 1348874 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Storage: Quota Manager, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v1])

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → shuang
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.
Whiteboard: [storage-v1]
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.
Attachment #8850529 - Flags: review?(jvarga)
Attachment #8850529 - Flags: review?(jvarga) → review+
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.
(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
I will study this promise question a bit more and update to this bug.
Ok, never mind then.
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.
Attachment #8856504 - Flags: review+
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2f6f1f7bd6
If origin is an opaque origin, reject promise with a TypeError, r=janv
This patch should be landed together with Bug 1286717.
https://hg.mozilla.org/mozilla-central/rev/aa2f6f1f7bd6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.