802544 - Plugin click-to-play "don't ask again for this site" checkbox does not work for file URIs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Santtu Lakkala]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010234852

Steps to reproduce:

I loaded an html file on sdcard (file:///sdcard/index.html) that uses a plugin via the <object> tag. When a pop-up appeared asking whether I wanted to run the plugin, ensured that the "don't show again" checkbox was checked and clicked "Yes"


Actual results:

Pretty much nothing happened. adb logcat shows a js error from nsIPermissionManager.add.


Expected results:

The plugin should be run, and potentially the pop-up should not appear for subsequent tries.

Comment 1

[Santtu Lakkala]

Attachment: Patch that removes the checkbox for URIs with no host

This is one way of fixing the bug. Simply don't show the checkbox for URIs that do not have a host.

Other way of fixing would be to alter permission manager to use something else as a key, if host is null.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch to modify permission manager

(In reply to Santtu Lakkala from comment #1)
> Other way of fixing would be to alter permission manager to use something
> else as a key, if host is null.

I think this is the right way to go.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Keeler from comment #2)
> I think this is the right way to go.

See bug 204285. Particularly bug 204285 comment 34, where sicking had some objections to this approach.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(also bug 546857 comment 86)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

So I think the best fix would be to take the front-end fix to just disable the menu item when it doesn't work, and leave the permission manager changes to another bug (bug 204285 I guess?).

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 672561 [details] [diff] [review]
patch to modify permission manager

Ok - we'll have to make the front-end changes in browser-plugins.js as well.

Comment 7

[:Margaret Leibovic]

Comment on attachment 672243 [details] [diff] [review]
Patch that removes the checkbox for URIs with no host

Sorry for the slow review! This looks good to me. Please also add a comment explaining that we need a valid uri.host to set a permission, so that's why we're doing this check.

Note to myself, this won't break the callback functions because aChecked will just be null instead of false:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5610

Comment 8

[Santtu Lakkala]

Attachment: Updated patch with descriptive comment

Patch with a comment =)

Comment 9

[:Margaret Leibovic]

Awesome, thanks!

I was going to check your patch in for you now, but the tree is closed because of build infrastructure issues. Since I can't land the patch right now, you could create a patch that's formatted to be checked in and practice using the checkin-needed keyword. See:
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 10

[Santtu Lakkala]

Attachment: Updated patch with correct format

Patch in better format.

Comment 11

[:Margaret Leibovic]

Thanks! I also added "r=margaret" to the end of the commit message, since all commit messages need to include a reviewer.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d857107b778

This will be merged to mozilla-central soon, and then work its way into Firefox 19.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d857107b778

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Now that bug 815640 has landed and file URIs can be used by the permission manager, maybe we should revert this.