1522912 - Consider preserving user gestures for rejected calls to Document.requestStorageAccess

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Steven Englehardt [:englehardt]]

Currently, we only preserve user gestures when the Promise returned by Document.requestStorageAccess is resolved. There are valid use cases for wanting to do so when the promise is rejected, such as allowing an embedded cross-origin frame to pop up a window that prompts the user to interact and login [0].

[0] https://github.com/whatwg/html/issues/3338#issuecomment-457373364

Comment 1

[(no longer active)]

Tooru, do you have any guidance for how to do this? For reference, we did this for resolve handlers in bug 1491403. Thanks!

Comment 2

[Tooru Fujisawa [:arai]]

Is this specific to the API, or in general the user gesture should be preserved for rejection?
If latter, passing aPropagateUserInteraction in Promise::Reject [1] will solve.

afaik whether the promise is rejected or not doesn't matter in the JS engine side.

[1] https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/dom/promise/Promise.cpp#142

Comment 3

[(no longer active)]

(In reply to Tooru Fujisawa [:arai] from comment #2)

Is this specific to the API, or in general the user gesture should be preserved for rejection?
If latter, passing aPropagateUserInteraction in Promise::Reject [1] will solve.

It is specific to this API. For this API, previously we ensured the resolve handlers are called with the user interaction flag set if it was set at the time the original Promise object was created. But we didn't do that for calling the reject handlers.

These promises are rejected in C++ using JS::RejectPromise(): https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/js/src/jsapi.cpp#4051.

afaik whether the promise is rejected or not doesn't matter in the JS engine side.

Do you mean the promises used in this API should already call their reject handlers with the user interaction flag set?

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to :Ehsan Akhgari from comment #3)

(In reply to Tooru Fujisawa [:arai] from comment #2)

Is this specific to the API, or in general the user gesture should be preserved for rejection?
If latter, passing aPropagateUserInteraction in Promise::Reject [1] will solve.

It is specific to this API. For this API, previously we ensured the resolve handlers are called with the user interaction flag set if it was set at the time the original Promise object was created. But we didn't do that for calling the reject handlers.

Okay, let me look into it tonight.

These promises are rejected in C++ using JS::RejectPromise(): https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/js/src/jsapi.cpp#4051.

afaik whether the promise is rejected or not doesn't matter in the JS engine side.

Do you mean the promises used in this API should already call their reject handlers with the user interaction flag set?

in JS engine side, the "user input" flag is copied to the Promise created with then,
regardless of fulfillment or rejection:
https://searchfox.org/mozilla-central/search?q=copyUserInteractionFlagsFrom&path=

DOM promise side controls the propagation, by calling JS::SetPromiseUserInputEventHandlingState.
https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/dom/promise/Promise.cpp#103-111

and the different between fulfillment/rejection is only in dom::Promise::{Resolve,Reject}.
https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/dom/promise/Promise.cpp#113-143

If this specific case doesn't propagate the user input only for rejection case, I think that uses dom::Promise::Rejectsomewhere.
and somehow overriding the flag would work.

Comment 5

[(no longer active)]

Actually you're right, I didn't know we handle resolve/reject exactly the same way. In this case we never call dom::Promise::Reject directly so we already do the right thing here. All that needs to be done is to just add a test case for this, I think.

Thanks for your help!

Comment 6

[(no longer active)]

Attachment: Bug 1522912 - Add a test case for propagation of user activation flag to the reject handler of promises returned from the Storage Access API;

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8abb91d7d1b7
Add a test case for propagation of user activation flag to the reject handler of promises returned from the Storage Access API; r=baku

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8abb91d7d1b7

Comment 9

[Chris Mills [:cmills]]

Note to MDN writers:

I've added a note to the Fx67 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/67#DOM

I'm not sure if anny other docs additions are necessary, but let's check.

Comment 10

[Steven Englehardt [:englehardt]]

Thanks Chris. I think we'll also need to update the Storage Access API MDN docs. Specifically here: https://developer.mozilla.org/en-US/docs/Web/API/Document/requestStorageAccess#Syntax. Would you mind to do that?

Comment 11

[Chris Mills [:cmills]]

(In reply to Steven Englehardt [:englehardt] from comment #10)

Thanks Chris. I think we'll also need to update the Storage Access API MDN docs. Specifically here: https://developer.mozilla.org/en-US/docs/Web/API/Document/requestStorageAccess#Syntax. Would you mind to do that?

Yup, we can do that when we finish off the docs task. No problem.

Comment 12

[Chris Mills [:cmills]]

I have updated this section to acocunt for the change:

https://developer.mozilla.org/en-US/docs/Web/API/Document/requestStorageAccess#Return_value

Let me know if you are happy with the wording. Thanks!

Comment 13

[Steven Englehardt [:englehardt]]

LGTM, thanks Chris.