Closed Bug 1187481 Opened 8 years ago Closed 5 years ago

Disable push quotas for installed apps

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: lina, Unassigned)

References

Details

Attachments

(3 files)

: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.
Bug 1187481 - Ignore uninitialized windows in `isTabOpen`. r=mt
Attachment #8638770 - Flags: review?(martin.thomson)
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
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.
Summary: Intermittent `TypeError: window.gBrowser is undefined` → Decide how to enforce push quotas for B2G apps
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).
Attachment #8638770 - Flags: review?(martin.thomson)
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.
Blocks: 1189091
Blocks: 1151180
Attachment #8638770 - Attachment description: MozReview Request: Bug 1187481 - Ignore uninitialized windows in `isTabOpen`. r=mt → MozReview Request: Bug 1187481, Part 1 - Use principals to test for push permissions. r?nsm
Attachment #8638770 - Flags: review?(nsm.nikhil)
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
Bug 1187481, Part 2 - Disable push quotas for installed apps. r?mt
Attachment #8647789 - Flags: review?(martin.thomson)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=291ce7a6d2b5
Summary: Decide how to enforce push quotas for B2G apps → Disable push quotas for installed apps
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 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)
Attachment #8647789 - Flags: review?(martin.thomson) → review+
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
Bug 1187481, Part 3 - Exclude open tabs from quotas on B2G. r?fabrice
Attachment #8647878 - Flags: review?(fabrice)
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 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!
Depends on: 1196512
(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.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Attachment #8647878 - Flags: review?(fabrice) → review+
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
Assignee: kcambridge → nobody
Status: ASSIGNED → NEW
Installed apps are no longer a thing, and part 1 landed in bug 1196512.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.