1491403 - Ensure the resolve handler of the promise returned by requestStorageAccess will have user activation (so that e.g. pages can open a popup from there)

Status: RESOLVED FIXED

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

Description

[(no longer active)]

WebKit's implementation had seen this feedback from developers.  We should ensure our implementation gets this right and add tests to that effect.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Our promises don't know about 'user activation' flag.

Comment 2

[(no longer active)]

I was thinking of manually calling EventStateManager::StartHandlingUserInput() and EventStateManager::StopHandlingUserInput() before and after MaybeResolveWithUndefined().  Would that not work?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I guess so, but rather hackish when we should just change the behavior consistently everywhere.

Comment 4

[(no longer active)]

We talked on IRC and decided to go with the hacky approach since the right fix here may take a while...  Filed bug 1494123 to remove the hack once the right fix arrives.

Comment 5

[(no longer active)]

The plan in comment 2 was *way* too simplistic, and it didn't work at all.  I should have guessed that.  The reason why is simple, because we need to push the user input event handling state at the time that we're running the promise resolve handler, aka the microtask checkpoint.

This means that we would actually need to teach our promises how to propagate the user input event handling state to some extent.  I have some patches that add some basic support for this.  This allows SpiderMonkey embedders to control a couple of bits on the promise that indicate whether the promise should propagate the user input event handling state, and whether we were handling a user input event when our promise was created.  This information is what is required for the microtask checkpoint code to be able to push the right user input event state before calling the promise resolve callback.

Furthermore, we also need to propagate these bits internally inside SpiderMonkey in Promise.then(), so that if the original promise had these bits set, the newly created promise returned from then() would also inherit them.

What is not clear is how to handle other similar cases, e.g. Promise.all(), Promise.race(), etc.  For now, we're going to punt on those (IOW, _not_ propagate these bits in any function besides Promise.then()) and leave that issue to bug 1185052.

I believe what is implemented here is sufficient for the use cases that the Storage Access API requires, which is mostly a promise chain resolve handler being able to do things like window.open().

Comment 6

[(no longer active)]

Attachment: Bug 1491403 - Part 1: Add a promise argument to JSEnqueuePromiseJobCallback

Comment 7

[(no longer active)]

Attachment: Bug 1491403 - Part 2: Expose SpiderMonkey APIs for indicating whether a promise must propagate user input event handling state

Depends on D7003

Comment 8

[(no longer active)]

Attachment: Bug 1491403 - Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it

Depends on D7004

Comment 9

[(no longer active)]

Attachment: Bug 1491403 - Part 4: Call the resolve handler of the promise returned from Document.requestStorageAccess() and Document.hasStorageAccess() preserving the user input event handling state

Depends on D7005

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 9012381 [details]
Bug 1491403 - Part 1: Add a promise argument to JSEnqueuePromiseJobCallback

Tooru Fujisawa [:arai] has approved the revision.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 9012384 [details]
Bug 1491403 - Part 4: Call the resolve handler of the promise returned from Document.requestStorageAccess() and Document.hasStorageAccess() preserving the user input event handling state

Andrea Marchesini [:baku] has approved the revision.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 9012383 [details]
Bug 1491403 - Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it

Andrea Marchesini [:baku] has approved the revision.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9012383 [details]
Bug 1491403 - Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it

Tooru, since you optimized Promise handling in bug 1317481, does this look ok for you? I'm a bit worried about if (!runnable->Initialize(aPromise, aJob)) { in CycleCollectedJSContext::EnqueuePromiseJobCallback

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 9012383 [details]
Bug 1491403 - Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it

Tooru Fujisawa [:arai] has approved the revision.

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 9012382 [details]
Bug 1491403 - Part 2: Expose SpiderMonkey APIs for indicating whether a promise must propagate user input event handling state

Tooru Fujisawa [:arai] has approved the revision.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9012383 [details]
Bug 1491403 - Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it

I think this patch is in my phabricator queue too so clearing the flag.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9012383 [details]
Bug 1491403 - Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ba3a8f9424d
Part 1: Add a promise argument to JSEnqueuePromiseJobCallback r=arai
https://hg.mozilla.org/integration/autoland/rev/b62dd5af680e
Part 2: Expose SpiderMonkey APIs for indicating whether a promise must propagate user input event handling state r=arai
https://hg.mozilla.org/integration/autoland/rev/18d88d24495a
Part 3: Propagate the user input event handling state to the promise resolve handlers in case the promise creator requests it r=smaug,arai,baku
https://hg.mozilla.org/integration/autoland/rev/ae1629a704aa
Part 4: Call the resolve handler of the promise returned from Document.requestStorageAccess() and Document.hasStorageAccess() preserving the user input event handling state r=baku

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2ba3a8f9424d
https://hg.mozilla.org/mozilla-central/rev/b62dd5af680e
https://hg.mozilla.org/mozilla-central/rev/18d88d24495a
https://hg.mozilla.org/mozilla-central/rev/ae1629a704aa

Comment 20

[Chris Mills [:cmills]]

I think I've documented this OK; I've made sure user activation is mentioned as a requirement in the main landing page, and I've also added a bit about needing user activation in the 
"Using" article: https://developer.mozilla.org/en-US/docs/Web/API/Storage_Access_API/Using

Does that work for you?

Comment 21

[(no longer active)]

Looks great, thanks!  I also added a bit of a prose to the Return value section of both hasStorageAccess and requestStorageAccess to explain that the resolve handler will run with user activation...