830983 - Remember my decision for desktop notification API in browser, clear storage data, access API again - API does not work

Status: VERIFIED FIXED

(Core :: Permission Manager, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 1/15/2013
Device: Unagi

Steps:

1. Go to http://jds2501.github.com/webapi-permissions-tests/webapi_permissions_test.html
2. Select the desktop notification API test and remember my choice
3. Go to browser settings and clear your storage data
4. Restart the browser
5. Repeat step #1 and step #2

Expected:

A desktop notification should appear.

Actual:

Nothing happens.

Additional Notes:

I'm generally noticing "weird" behavior happening when you clear your storage data and try to reuse an existing site/WebAPI. What's really bad here is that clearing your storage data is supposed help you start with a clean state, but in this case, you are stuck unless you factory reset the phone.

Comment 1

[Jason Smith [:jsmith]]

So there's some "strange" behavior happening as a result of clearing storage data. Mounir - Any ideas on why there's some weird behavior going on here?

Comment 2

[Jason Smith [:jsmith]]

Attachment: Logcat

Comment 3

[Jason Smith [:jsmith]]

Looks like there may be more than one bug here. I'm seeing really not so nice things in the logcat after trying to use the browser after clearing storage data.

01-15 13:52:33.349: E/GeckoConsole(587): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [inIDOMUtils.setContentState]" {file: "chrome://global/content/BrowserElementChild.js" line: 1254}]

01-15 13:52:52.619: E/GeckoConsole(109): [JavaScript Error: "NO SETTINGS PERMISSION FOR: app://browser.gaiamobile.org

01-15 13:53:18.779: E/GeckoConsole(109): [JavaScript Error: "TypeError: this.currentTab.dom.reload is not a function" {file: "app://browser.gaiamobile.org/js/browser.js" line: 597}]

Comment 4

[Ben Francis [:benfrancis]]

Those logcat messages hint that the browser app has lost all of its permissions including settings and access to the Browser API.

Clearing private data in the browser should clear permissions for browser content, but not the browser app itself.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 830983: Prevent removing in-memory app permissions when deleting only its in-browser permissions.

Comment 6

[Alexandre Poirot [:ochameau]]

Ben, I'm facing same kind of issues in bug 829397, could you give a try to this platform patch and see if it fix your issue without breaking permission cleaning?

Comment 7

[Justin Lebar (not reading bugmail)]

FWIW that patch looks right to me, but we should get Mounir to look at it.

Comment 8

[Lucas Adamski [:ladamski]]

Can't be permanently breaking permissions.

Comment 9

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

Comment on attachment 702787 [details] [diff] [review]
Bug 830983: Prevent removing in-memory app permissions when deleting only its in-browser permissions.

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

Trivial fix. I'll mark this one since Mounir is traveling.

Comment 10

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa20a6097d0b

Comment 11

[Andrea Marchesini [:baku]]

Backed out. OTH test failed: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=aa20a6097d0b

https://hg.mozilla.org/integration/mozilla-inbound/rev/3702b7ead05e

Comment 12

[Mounir Lamouri (:mounir)]

Isn't that a regression from bug 827050 where we are removing stuff from the DB but not from the memory representation of the PermissionManager? The patch was clearly wrong (if aBrowserOnly=false, we remove no permissions...) and was a hack anyway because we should have change GetPermissionsForApp (the enumerator).

Taking this.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Hmm I would have put the      

data.permissions[i]->GetHost(host);

and

data.permissions[i]->GetType(type);

after the continue to avoid getting data we're not going to use. But other than that the patch looked correct. If aBrowserOnly=false then all permissions will be removed (which is the uninstall app use) otherwise only the ones that have isInBrowserElement=true will be removed (which are the same that are being deleted from the database).

Are you sure this isn't a test failure (as in the test not being correct, not the function being incorrect)?

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 15

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4886df89ef45
https://hg.mozilla.org/releases/mozilla-b2g18/rev/28756e5bb960

Comment 16

[Jason Smith [:jsmith]]

Resolved fixed per jst rules - inbound + b2g18.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/4886df89ef45

Comment 18

[Jason Smith [:jsmith]]

Verified on 1/20 build.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.