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

NEW
Unassigned

Status

()

Core
DOM
4 years ago
4 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

This needs to always call back synchronously. The specific function seems to be handleExistingPermission(). B2G and Android both have this problem, maybe others.
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #0)
> This needs to always call back synchronously.

Bah. Always *asynchronously*.

Comment 2

4 years ago
Created attachment 823170 [details] [diff] [review]
bug_927521_webrt
Attachment #823170 - Flags: review?

Comment 3

4 years ago
Created attachment 823171 [details] [diff] [review]
bug_927521_mock

Updated

4 years ago
Attachment #823171 - Flags: review?

Comment 4

4 years ago
Created attachment 823172 [details] [diff] [review]
bug_927521_metro
Attachment #823172 - Flags: review?(mark.finkle)

Comment 5

4 years ago
Created attachment 823173 [details] [diff] [review]
bug_927521_b2g
Attachment #823173 - Flags: review?(fabrice)

Comment 6

4 years ago
Created attachment 823174 [details] [diff] [review]
bug_927521_android
Attachment #823174 - Flags: review?(blassey.bugs)

Comment 7

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

Updated

4 years ago
Attachment #823171 - Flags: review? → review?(jmaher)
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.
Attachment #823173 - Flags: review?(fabrice) → feedback+
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
Attachment #823174 - Flags: review?(blassey.bugs) → review?(mark.finkle)
Attachment #823170 - Flags: review? → review?(mark.finkle)
(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.
Attachment #823171 - Flags: review?(jmaher) → review+
* 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.
(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 on attachment 823170 [details] [diff] [review]
bug_927521_webrt

want new patch
Attachment #823170 - Flags: review?(mark.finkle) → feedback+
Comment on attachment 823172 [details] [diff] [review]
bug_927521_metro

want new patch
Attachment #823172 - Flags: review?(mark.finkle) → feedback+
Comment on attachment 823174 [details] [diff] [review]
bug_927521_android

want new patch
Attachment #823174 - Flags: review?(mark.finkle) → feedback+
(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.
srsly.

mfinkle, please readdress your comments.  if you want to write the patches, that would be doubleplus good.
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
request.allow.bind(request) springs to mind.
(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?

Updated

4 years ago
Assignee: doug.turner → nobody
Component: General → DOM
Product: Firefox OS → Core
You need to log in before you can comment on or make changes to this bug.