Open Bug 927521 Opened 9 years ago Updated 2 years ago

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

Categories

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

defect

Tracking

()

People

(Reporter: bent.mozilla, Unassigned)

References

Details

Attachments

(5 files)

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*.
Attached patch bug_927521_webrtSplinter Review
Attachment #823170 - Flags: review?
Attachment #823171 - Flags: review?
Attached patch bug_927521_metroSplinter Review
Attachment #823172 - Flags: review?(mark.finkle)
Attached patch bug_927521_b2gSplinter Review
Attachment #823173 - Flags: review?(fabrice)
Attachment #823174 - Flags: review?(blassey.bugs)
I've tested these changes on FF desktop (replacing all calls to allow() or cancel() with postPromptResponse()).  I have not tested your platform.
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?
Assignee: doug.turner → nobody
Component: General → DOM
Product: Firefox OS → Core
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.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
See Also: → 933825
You need to log in before you can comment on or make changes to this bug.