Closed Bug 802544 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 19

People

(Reporter: inz, Assigned: inz)

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Attachment #672243 - Flags: review?(margaret.leibovic)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
Attachment #672561 - Flags: review?(mconnor)
(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.
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 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.
Attachment #672561 - Attachment is obsolete: true
Attachment #672561 - Flags: review?(mconnor)
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
Attachment #672243 - Flags: review?(margaret.leibovic) → review+
Patch with a comment =)
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
Patch in better format.
Attachment #672243 - Attachment is obsolete: true
Attachment #675480 - Attachment is obsolete: true
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.
Assignee: nobody → inz
OS: Linux → Android
Hardware: x86_64 → ARM
Target Milestone: --- → Firefox 19
https://hg.mozilla.org/mozilla-central/rev/1d857107b778
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Now that bug 815640 has landed and file URIs can be used by the permission manager, maybe we should revert this.
You need to log in before you can comment on or make changes to this bug.