1617893 - already granted notification permissions not detected if page in container tab

Status: VERIFIED FIXED

(Core :: Permission Manager, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 75.0a1 20200225094028 on Windows 8.1

Slack shows its own notification bar that one shall grant it permissions to show notifications despite already having granted it. Clicking the link to grant it doesn't show the notification prompt/doorhanger.

Steps to reproduce:

1. Open https://mozilla.slack.com
2. Try to grant notification permissions from inside the website.

If the issues cannot be reproduced with step 1 and 2, first ran an older Firefox Nightly and grant the notification permission with that before updating to an affected build.

Bisection points to bug 1614831 as being the cause.

Comment 1

[Johann Hofmann [:johannh]]

Ehsan, from the symptoms described by people on Slack it feels like the Permissions API (or the whole content process) has issues getting the right permissions, while the parent can still correctly get and display them. Can you look into this issue? :)

Comment 2

[(no longer active)]

[Tracking Requested - why for this release]: major web-facing regression

Comment 3

[(no longer active)]

It seems like the slack link doesn't work on my development machine because auth0 is somehow broken there. Better test case: https://mdn.mozillademos.org/en-US/docs/Web/API/Notifications_API/Using_the_Notifications_API$samples/Tag_example?revision=1606062

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The permissions work with the link from comment 3 here (not seeing a notification can also have different reasons, see e.g. bug 1337490).

Comment 5

[(no longer active)]

When the bug happens we're coming from TransmitPermissionsForPrincipal() and calling EnsurePermissionsByKey("https://app.slack.com^userContextId=2", "https://app.slack.com"). Then we get to nsPermissionManager::GetPermissionsFromOriginOrKey() with that origin string (which has the user context ID not stripped)) and the origin comparison inside that function fails.

The fix is very simple, but writing a test for this took much longer than I hoped for. I'll submit the full patch tomorrow.

Comment 6

[(no longer active)]

I spent several hours trying to write a test for it which would fail without the fix and pass with it, and I think I'm going to give up now. The reason why it is so difficult to do so is that for this test to work we would need TransmitPermissionsAndBlobURLsForPrincipalInfo() to be the first caller to ContentParent::TransmitPermissionsForPrincipal() after the content process has been launched such that we get around this cache.

I burned way too much time trying to create a test for this in order to replicate this exact condition as a mochitest and it seems to be impossible to do so. Hereby I declare defeat, sorry about that. :/

Comment 7

[(no longer active)]

Attachment: Bug 1617893 - Ensure that we properly strip OriginAttributes when deciding which permissions to send for a principal;

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38dd2cbee2fe
Ensure that we properly strip OriginAttributes when deciding which permissions to send for a principal; r=baku

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/38dd2cbee2fe

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Verified fixed with Nightly 75.0a1 20200227094956 on Windows 8.1