1187481 - Disable push quotas for installed apps

Status: RESOLVED WONTFIX

(Core :: Storage: IndexedDB, defect)

Description

[Lina Butler [:lina]]

:robertbindar saw this lovely error in the console while trying to get Push working in Nightly: "receivedPushMessage: Error notifying app: TypeError: window.gBrowser is undefined".

We haven't been able to reproduce it, and I wonder what kind of chrome window wouldn't have a `gBrowser` object...but, either way, we shouldn't crash with an exception if we can't determine whether a tab is open.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm

Bug 1187481 - Ignore uninitialized windows in `isTabOpen`. r=mt

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8638770 [details]
MozReview Request: Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm

Bug 1187481 - Ignore uninitialized windows in `isTabOpen`. r=mt

Comment 3

[Lina Butler [:lina]]

Ah, I missed that this happens on B2G, not Desktop. In that case, this patch is insufficient. We'll want to think about how installed apps interact with the quota, assuming we want to enforce it.

Comment 4

[Martin Thomson [:mt:]]

Maybe disable push for installed apps (at least those with the app:// scheme).  We're moving to using https:// schemes for "installed" apps, which probably won't have this problem.  I'd check with someone on the B2G architecture team first though (:sicking perhaps).

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8638770 [details]
MozReview Request: Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm

https://reviewboard.mozilla.org/r/14121/#review12751

::: dom/push/PushRecord.jsm:153
(Diff revision 2)
> +    } catch (e) {
> +      // Bug 1187481: Some chrome windows may not have a browser property.
> +      // Ignore this, and any other exceptions; `getLastVisit` will fall back
> +      // to the Places query.
> +      return false;
> +    }

rather than testing window.gBrowser you can test ("gBrowser" in window) and avoid all this muck.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8638770 [details]
MozReview Request: Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm

Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm

Comment 7

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1187481, Part 2 - Disable push quotas for installed apps. r?mt

Bug 1187481, Part 2 - Disable push quotas for installed apps. r?mt

Comment 8

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=291ce7a6d2b5

Comment 9

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/14121/#review14377

::: dom/push/PushRecord.jsm:160
(Diff revision 3)
> +  pushPermission() {

I kept `pushPermission` and `hasPermission` separate to minimize churn between this and bug 1192458.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8647789 [details]
MozReview Request: Bug 1187481, Part 2 - Disable push quotas for installed apps. r?mt

https://reviewboard.mozilla.org/r/16041/#review14387

LGTM

::: dom/push/PushRecord.jsm:164
(Diff revision 1)
> +        // B2G, or a chrome window without a tab browser.
> +        continue;

Either this results in only installed apps working on B2G, or your comment isn't right.

::: dom/push/test/xpcshell/test_permissions.js:25
(Diff revision 1)
> +  addPermission('https://example.com/site', 'ALLOW_ACTION');
> +  let record = new PushRecord({ scope: 'https://example.com/site' });

reorder (record is the fixture here)

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8647789 [details]
MozReview Request: Bug 1187481, Part 2 - Disable push quotas for installed apps. r?mt

Bug 1187481, Part 2 - Disable push quotas for installed apps. r?mt

Comment 12

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1187481, Part 3 - Exclude open tabs from quotas on B2G. r?fabrice

Bug 1187481, Part 3 - Exclude open tabs from quotas on B2G. r?fabrice

Comment 13

[Lina Butler [:lina]]

Fabrice, could you have a look at the `getTabURIs` function, please? It's used to exempt sites with open, non-private tabs from the push quota.

I tried it out in the simulator; not sure how to write a real test given the `SystemAppProxy` dependency...

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8638770 [details]
MozReview Request: Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm

https://reviewboard.mozilla.org/r/14121/#review14439

Ship It!

Comment 15

[[:fabrice] Fabrice Desré]

(In reply to Kit Cambridge [:kitcambridge] from comment #13)
> Fabrice, could you have a look at the `getTabURIs` function, please? It's
> used to exempt sites with open, non-private tabs from the push quota.
> 
> I tried it out in the simulator; not sure how to write a real test given the
> `SystemAppProxy` dependency...

You can write a b2g mochitest. Did you test on device? I'm not 100% sure that frame.contentWindow will give you what you need with OOP frames.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8647878 [details]
MozReview Request: Bug 1187481, Part 3 - Exclude open tabs from quotas on B2G. r?fabrice

https://reviewboard.mozilla.org/r/16073/#review16725

Comment 17

[Lina Butler [:lina]]

Installed apps are no longer a thing, and part 1 landed in bug 1196512.