889835 - Use SitePermissions.jsm as the backend for Page Info's Permissions tab

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Lots of custom cruft remains for the plugin and indexedDB permissions, unfortunately, but that doesn't affect other permissions, where all that's needed now is the gPermissionObject entry and the label in sitePermissions.properties.

There's also some potential to teach SitePermissions.jsm about plugin permissions such that they'd be exposed in the identity panel as well, but I'm not going to worry about that in this bug.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 770808 [details] [diff] [review]
patch

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

>+    onChange: function (aURI, aState) {
>       if (aState == SitePermissions.ALLOW || aState == SitePermissions.BLOCK)
>         Services.perms.remove(aURI.host, "indexedDB-unlimited");
>     }

I see that you didn't introduce this, but I spent a half hour looking through commits and bug comments trying to understand this. Is "indexedDB-unlimited" something that is being migrated away from? Following the code path here, during .set(), we call Services.perms.add(aURI.host, "indexedDB") and then subsequently call Services.perms.remove(aURI.host, "indexedDB-unlimited") within this function. I couldn't find any place that re-adds the "indexedDB-unlimited" permission. Do you understand this better than I do? If so, can you please share some background on this and add a comment to this block of code?

::: browser/base/content/pageinfo/permissions.js
@@ +183,5 @@
>  {
> +  let row = document.getElementById("perm-indexedDB-row");
> +  let extras = document.getElementById("perm-indexedDB-extras");
> +
> +  row.appendChild(extras);

perm-indexedDB-extras should probably have hidden="true" until it is appended to this row.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Ben, can you provide a response to my questions in comment #2 since you introduced this code?

Comment 4

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #2)
> I see that you didn't introduce this, but I spent a half hour looking
> through commits and bug comments trying to understand this. Is
> "indexedDB-unlimited" something that is being migrated away from? Following
> the code path here, during .set(), we call Services.perms.add(aURI.host,
> "indexedDB") and then subsequently call Services.perms.remove(aURI.host,
> "indexedDB-unlimited") within this function. I couldn't find any place that
> re-adds the "indexedDB-unlimited" permission. Do you understand this better
> than I do? If so, can you please share some background on this and add a
> comment to this block of code?

http://hg.mozilla.org/mozilla-central/annotate/a0c7e9f6c6be/dom/quota/CheckQuotaHelper.cpp#l170 adds it and webapprt/Startup.jsm, but I'm not sure if that uses the same profile as desktop Firefox.

> perm-indexedDB-extras should probably have hidden="true" until it is
> appended to this row.

Seems unnecessary since this code should always run when the window opens.

Comment 5

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcb8d073908

Comment 6

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

We have two permissions that govern indexedDB behavior:

1. "indexedDB" determines whether or not the given site has permission to use indexedDB at all. If no particular permission was set then the default *used to be* to prompt the user with a door-hanger. Web authors hated this approach so we changed it, probably in a confusing way. Now when no permission is set we allow use without a prompt. If the permission is set to "deny" then we do not allow access. If the permission is set to "allow" then we prompt. We still have three states but the default has now changed.

2. "indexedDB-unlimited" determines how much space is available for use. If no permission is set, or if the permission is set to "deny", then the site is allowed a small amount of space. Once the space is exhausted we may present a door-hanger prompt if the permission is still unset to extend the quota. If the permission is set to "allow" then the site can use an unlimited amount of space.

When changing the "indexedDB" permission (to "allow" or to "deny") we need to remove the "indexedDB-unlimited" permission in order to match user expectations.

Hope that helps. I realize this is a bit backwards but the permission interface is a bit broken imo...

Comment 7

[Dão Gottwald [:dao]]

(In reply to ben turner [:bent] from comment #6)
> 1. "indexedDB" [...] Now when no
> permission is set we allow use without a prompt.
> [...] If the permission is set to "allow" then
> we prompt.

I'm not sure I understand this part, it doesn't seem to make any sense. Could you please check the page info window with this patch applied and see if it still matches your expectations?

Comment 8

[Dão Gottwald [:dao]]

And please file a bug if it doesn't. Thanks!

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

I'm flagging Ben for needinfo now to make sure that he sees your follow-up question.

Comment 10

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

(In reply to Dão Gottwald [:dao] from comment #7)
> I'm not sure I understand this part, it doesn't seem to make any sense.

What don't you understand? I can try to make it more clear but it seems pretty clear to me.

> Could you please check the page info window with this patch applied and see
> if it still matches your expectations?

Did you change the indexedDB code here? I looked briefly and didn't see the indexedDB changes.

Comment 11

[Dão Gottwald [:dao]]

(In reply to ben turner [:bent] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > I'm not sure I understand this part, it doesn't seem to make any sense.
> 
> What don't you understand? I can try to make it more clear but it seems
> pretty clear to me.

I don't understand why we'd prompt when the permission says "allow" while not prompting with no permission stored.

> > Could you please check the page info window with this patch applied and see
> > if it still matches your expectations?
> 
> Did you change the indexedDB code here? I looked briefly and didn't see the
> indexedDB changes.

The change is that SitePermissions.jsm controls which permissions to expose in the UI (e.g. Always Ask / Allow / Block) and what the default state is.

Comment 12

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

(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to ben turner [:bent] from comment #10)
> I don't understand why we'd prompt when the permission says "allow" while
> not prompting with no permission stored.

I explained that how this changed above. The default (no permission set) is now to allow indexedDB use. The "allow" permission is used to indicate that we need to prompt. I agree that it is confusing, but that is how the code works currently.

> The change is that SitePermissions.jsm controls which permissions to expose
> in the UI (e.g. Always Ask / Allow / Block) and what the default state is.

It's still not clear to me whether or not you changed the behavior of the indexedDB items. If you did then they're probably now broken. If you didn't then they're probably not :)

Comment 13

[Dão Gottwald [:dao]]

(In reply to ben turner [:bent] from comment #12)
> It's still not clear to me whether or not you changed the behavior of the
> indexedDB items.

I tried to maintain existing behavior as much as it seemed to make sense. But since you're telling me that indexedDB wasn't supposed to make sense in the first place, I have no idea anymore if it's broken or not. I hoped you could tell by looking at the actual UI.

Comment 14

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

(In reply to Dão Gottwald [:dao] from comment #13)
> I hoped you could tell by looking at the actual UI.

I'm pretty swamped at a work week right now so it will take some time. Sounds like you might want to back out this change in the meantime though.

Comment 15

[Dão Gottwald [:dao]]

Opening the page info window and looking at the available states and the default state for indexedDB should only take a minute, maybe some more minutes if you want to play around with it on a page actually using that permission. But it's ok, take your time. It will be four weeks until this hits aurora. I'll just flag you for needinfo such that you hopefully won't forget about it.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bcb8d073908

Comment 17

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

Can we please not make it my responsibility to ensure that you didn't break something? You should verify this yourself or just back it out.

Go to http://robnyman.github.io/html5demos/indexeddb/index.html and you should not get a prompt. Then use the Page Info dialog and make sure that all of the different permission modes work with the page as expected.

Comment 18

[Dão Gottwald [:dao]]

First of all, it's not my fault that indexedDB is abusing nsIPermissionManager. Having said that, I didn't ask you to ensure things don't break, I'm happy to take any follow-up bugs. I merely asked for your help to figure out whether something is broken. I can try to figure this out myself, but there's no guarantee that I won't miss something, given the mess (that I didn't cause).

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Dão, it is absolutely your responsibility to ensure that a patch that you pushes does not break our product. That responsibility doesn't just extend to writing the needed patches. It also includes figuring out what patches are needed.

The fact that the IndexedDB code does something in a hacky way is not an excuse to break it. The hack is there because nsIPermissionManager didn't at the time have the feature set that we needed.

It's great that you are asking for help. But ultimately it's not the responsibility of others to check that your patches doesn't break them.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Jonas Sicking (:sicking) from comment #19)
> The fact that the IndexedDB code does something in a hacky way is not an
> excuse to break it.

I don't know what, if anything, is broken. I will try to find out, as I said in comment 18.

Comment 21

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

I understand that this code is messy and I am totally sympathetic. I was just saying that this code has existed in its present form for a little over a year. It worked then and now we don't know. I don't think it's ok to check something in that we aren't sure works when we haven't tried to verify it yet.

Comment 22

[Dão Gottwald [:dao]]

If it's broken, it was already broken in bug 885366. This bug made us use SitePermissions.jsm in more places, but it needs to work correctly either way.