927521 - ContentPermissionPrompt in at least two products sometimes calls back asynchronously and sometimes calls back synchronously

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This needs to always call back synchronously. The specific function seems to be handleExistingPermission(). B2G and Android both have this problem, maybe others.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #0)
> This needs to always call back synchronously.

Bah. Always *asynchronously*.

Comment 2

[Doug Turner (:dougt)]

Attachment: bug_927521_webrt

Comment 3

[Doug Turner (:dougt)]

Attachment: bug_927521_mock

Comment 4

[Doug Turner (:dougt)]

Attachment: bug_927521_metro

Comment 5

[Doug Turner (:dougt)]

Attachment: bug_927521_b2g

Comment 6

[Doug Turner (:dougt)]

Attachment: bug_927521_android

Comment 7

[Doug Turner (:dougt)]

I've tested these changes on FF desktop (replacing all calls to allow() or cancel() with postPromptResponse()).  I have not tested your platform.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 823173 [details] [diff] [review]
bug_927521_b2g

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

::: b2g/components/ContentPermissionPrompt.js
@@ +105,5 @@
> +                request.cancel();
> +            }
> +        }
> +    }, Ci.nsIThread.DISPATCH_NORMAL);
> +}

nit: 2 spaces indent please.
less nitty: instead of (request, allow) you could just pass the function as a parameter, that would simplify the whole thing.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 823174 [details] [diff] [review]
bug_927521_android

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

JS code largely makes me confused. over to finkle

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Fabrice Desré [:fabrice] from comment #8)

> nit: 2 spaces indent please.
> less nitty: instead of (request, allow) you could just pass the function as
> a parameter, that would simplify the whole thing.

I would go further and ask:
* Why doesn't the nsIContentPermissionRequest implementor handle this async? 
* Why does the JS front-end code need to worry about it?
* Is the API broken in a "we didn't think about async" kinda way?
* Is this just a bandaid fix needed for a quick release?

Having the JS side do this little thread trick is not a good idea.

Comment 11

[Doug Turner (:dougt)]

* Why doesn't the nsIContentPermissionRequest implementor handle this async? 

because there are more of those than the dialoging implementation.

* Why does the JS front-end code need to worry about it?

both sides -- the request and the dialog -- are in js.

* Is the API broken in a "we didn't think about async" kinda way?

Yes. ;/

* Is this just a bandaid fix needed for a quick release?

Yes, but we probably will never 'really fix it' and change the api.  Priority thing -- we have better things to do.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Doug Turner (:dougt) from comment #11)

> * Is the API broken in a "we didn't think about async" kinda way?
> 
> Yes. ;/
> 
> * Is this just a bandaid fix needed for a quick release?
> 
> Yes, but we probably will never 'really fix it' and change the api. 
> Priority thing -- we have better things to do.

I am gripped with sadness. 

Let's get some new patches with Fabrice's suggestions in comment 8 and a simple name change:

>+  _postPromptResponse: function(request, allow) {

nit: name change
not-nit: pass in the action

_postResponse: function(action)

>+      Services.tm.mainThread.dispatch({
>+          run: function() {
>+              if (allow) {
>+                  request.allow();
>+              } else {
>+                  request.cancel();
>+              }
>+          }

by passing in the action as a function, this should become:

>          run: function() {
>            action();
>          }

And then you when you call it:

>-      request.allow();
>+      postPromptResponse(request, true);

Do this:

>      postResponse(request.allow);

I think that should work. (note no parens!)

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 823170 [details] [diff] [review]
bug_927521_webrt

want new patch

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 823172 [details] [diff] [review]
bug_927521_metro

want new patch

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 823174 [details] [diff] [review]
bug_927521_android

want new patch

Comment 16

[Josh Matthews [:jdm]]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> (In reply to Doug Turner (:dougt) from comment #11)
> >+  _postPromptResponse: function(request, allow) {
> 
> nit: name change
> not-nit: pass in the action
> 
> _postResponse: function(action)
> 
> >+      Services.tm.mainThread.dispatch({
> >+          run: function() {
> >+              if (allow) {
> >+                  request.allow();
> >+              } else {
> >+                  request.cancel();
> >+              }
> >+          }
> 
> by passing in the action as a function, this should become:
> 
> >          run: function() {
> >            action();
> >          }
> 
> And then you when you call it:
> 
> >-      request.allow();
> >+      postPromptResponse(request, true);
> 
> Do this:
> 
> >      postResponse(request.allow);
> 
> I think that should work. (note no parens!)

This is not the same, since the action function is no longer bound to the correct this value.

Comment 17

[Doug Turner (:dougt)]

srsly.

mfinkle, please readdress your comments.  if you want to write the patches, that would be doubleplus good.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Josh Matthews [:jdm] from comment #16)

> > Do this:
> > 
> > >      postResponse(request.allow);
> > 
> > I think that should work. (note no parens!)
> 
> This is not the same, since the action function is no longer bound to the
> correct this value.

Let's find a way to make it work. It's JS, we can do many amazing things.

We need some comments in the code to let others know what the heck is happening and why. Also, the IDL comments for nsIContentPermissionPrompt.prompt need to be updated to be very explicit about this requirement. We might have add-ons that are affected too.

Comment 19

[Josh Matthews [:jdm]]

request.allow.bind(request) springs to mind.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Josh Matthews [:jdm] from comment #19)
> request.allow.bind(request) springs to mind.

What about

this._postResponse(function() request.allow());

(using my old method of passing a function into the _postResponse method)

It's kinda like:

this._postResponse(function() {
  request.allow()
});

Only shorter. And the anon function creates a closure on the request, right?

Comment 21

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.