1279622 - Regression: modal windows under about:preferences within a container are broken

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: example1.png

Modal windows that are opened from about:preferences within a container have their theme/UI stripped. Opening the cookie manager will produce the following warnings within the terminal:

[Parent 98241] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Users/kjozwiak/projects/m-c-containers/caps/nsScriptSecurityManager.cpp, line 1052
[Parent 98241] WARNING: robustness marked as unsupported: file /Users/kjozwiak/projects/m-c-containers/gfx/gl/GLContextFeatures.cpp, line 907

If you close the cookie manager window and attempt to re-open it within the same container, the window won't open and you'll see the following warning in the terminal:

[Parent 98241] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Users/kjozwiak/projects/m-c-containers/caps/nsScriptSecurityManager.cpp, line 1052

It looks like bug # 1253538 has caused this regression.

STR:

* launch the latest version of m-c
* enable containers via about:config -> privacy.userContext.enabled;true
* open a new container via file -> new container tab -> personal
* visit about:preferences#privacy and click on "remove individual cookies." (notice the theme/UI being stripped)
* close and attempt to re-open the cookie manager

Regression Range:

29:33.99 INFO: Last good revision: b380eca3966c37ed9a32c990c4991cf622e3dfde
29:33.99 INFO: First bad revision: 0c2cf128ceae933dd54fb078dbf69ee3c0f54cc7
29:33.99 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b380eca3966c37ed9a32c990c4991cf622e3dfde&tochange=0c2cf128ceae933dd54fb078dbf69ee3c0f54cc7

29:34.90 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1253538

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Attachment: example2.png

Comment 2

[Kamil Jozwiak [:kjozwiak]]

Attachment: correctExample.png

Example of how the cookie manager modal window within about:preferences should look like.

Comment 3

[Andrea Marchesini [:baku]]

Do we want to allow about: pages to run in containers?
I think it's confusing for the user. about:preferences should always open in a userContextId=0 tab.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini (:baku) from comment #3)
> Do we want to allow about: pages to run in containers?
> I think it's confusing for the user. about:preferences should always open in
> a userContextId=0 tab.

That sounds right, but then what do you do if a user opens about:preferences in a container tab?  Open a new tab with about:preferences in it?

You can already observer similar behavior if you open a tab with about;preferences, and then open another tab (of any type) that tries to access about;preferences.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #4)
> You can already observer similar behavior if you open a tab with
> about;preferences, and then open another tab (of any type) that tries to
> access about;preferences.
Actually that's not true exactly true.  If you type in about:preferences in the url bar, you can see it in multiple tabs.  If you use the Menu item for it, then it takes you back to the tab you already have it open in.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - Opening about:preferences from the menu should not use a container tab.

Comment 7

[Andrea Marchesini [:baku]]

Bram, I need a feedback from you. What about if we don't allow the user to open about:preferences (and maybe some other about: pages) from a container tab? We could do something similar to what we do when the user opens about:privatebrowsing from a non private browsing session.

I already uploaded a patch here. With this patch we open about:something pages in a new default tab also if there is an available container empty tab. The question is about the user, manually opening about:preferences. Thanks.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8767714 [details] [diff] [review]
part 1 - Opening about:preferences from the menu should not use a container tab.

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

::: browser/base/content/browser.js
@@ +7410,5 @@
>      for (let i = 0; i < browsers.length; i++) {
>        let browser = browsers[i];
> +
> +      // Container tabs must always be ignored.
> +      if (browser.hasAttribute('usercontextid')) {

Nit - double-quotes is more common in here.

@@ +7462,5 @@
>  
>    // No opened tab has that url.
>    if (aOpenNew) {
> +    if (isBrowserWindow && isTabEmpty(gBrowser.selectedTab) &&
> +        !gBrowser.selectedTab.hasAttribute('usercontextid'))

Nit: double-quotes

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8767714 [details] [diff] [review]
part 1 - Opening about:preferences from the menu should not use a container tab.

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

Actually, won't this break the switch-to-tab functionality in the AwesomeBar for container tabs?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #9)
> Comment on attachment 8767714 [details] [diff] [review]
> part 1 - Opening about:preferences from the menu should not use a container
> tab.
> 
> Review of attachment 8767714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, won't this break the switch-to-tab functionality in the AwesomeBar
> for container tabs?

(I just checked, and yes it will).

Comment 11

[Bram Pitoyo [:bram]]

(In reply to Andrea Marchesini (:baku) from comment #7)
> Bram, I need a feedback from you. What about if we don't allow the user to
> open about:preferences (and maybe some other about: pages) from a container
> tab? We could do something similar to what we do when the user opens
> about:privatebrowsing from a non private browsing session.
> 
> I already uploaded a patch here. With this patch we open about:something
> pages in a new default tab also if there is an available container empty
> tab. The question is about the user, manually opening about:preferences.
> Thanks.

When you type about:privatebrowsing in normal mode, there’s a page that helpfully directs you to switch mode into PBM.

Can we do the same here? We can show a page that says “Preferences cannot open in a Container Tab. [Open Preferences in a New Tab]”.

Alternatively, when you type about:preferences in a container tab, we can magically switch that tab to a normal tab, then open the Preferences UI there. But is this possible?

Thoughts?

Comment 12

[:Gijs (he/him)]

As I said to baku on IRC, seems like about:privatebrowsing is pretty different in that it would be actively misleading to show the 'private browsing' content in non-privatebrowsing. If we can make about:preferences "Just Work" in container tabs that would be better (and AIUI, it does 'just work' beside the subdialogs, which should be fixable).

Comment 13

[Andrea Marchesini [:baku]]

Attachment: docshell.patch

The issue here is that the comparison of docShell originAttributes fails. the accessing DS uses a system principal and UCI==1, but the target doesn't have a system principal and UCI==0.
I don't know if this is the right fix.

Comment 14

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #13)
> the target doesn't have a system principal and UCI==0.

Why doesn't it? The things we load in the dialog frame have the system principal... This seems like it indicates the frame gets created with the wrong UCI if the UCI of the parent is non-0...

Comment 15

[Tanvi Vyas[:tanvi]]

When the user opens about:preferences in a container tab, a new tab should open in the default container and about:preferences should open there.  This gives users a hint that preferences are not tied to containers.  

Otherwise, a user may open a banking container and go to about:preferences.  They will see the container Ui indicators and disable third party cookies, thinking this is just for the Banking Container, when it is in fact for all containers.

If the user closes the about:preferences tab or hits the back button, we can close the default container tab and go back to the Banking container.

Comment 16

[:Gijs (he/him)]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #15)
> Otherwise, a user may open a banking container and go to about:preferences. 
> They will see the container Ui indicators and disable third party cookies,
> thinking this is just for the Banking Container, when it is in fact for all
> containers.

Feels like we should just remove the container UI when this happens, and put them back when we navigate away. AIUI from baku on IRC, about:preferences doesn't have a usercontextid associated anyway because it's chrome-privileged. Though in any case, I'm not very convinced that it's likely people will think that all the options in about:preferences are per-container. :-\

> If the user closes the about:preferences tab or hits the back button, we can
> close the default container tab and go back to the Banking container.

Which about:preferences tab, the one they opened or the one we cloned into a default container tab? What happens if they open a different page in that tab, then go back to about:preferences? What if there was other history in the tab beforehand and now we're closing the tab (or users go back, and then forward again)? Adding manual work to all the ways in which the user could possibly close or navigate away from a page seems super-complicated and not very much in line with the severity of the issue at hand...

Comment 17

[Tanvi Vyas[:tanvi]]

If they don't close the default container tab with about:preferences in it, they can use it and navigate to a page.  But that may also cause issues, since they were operating in the Banking context, and now they are in the default context without realizing it.

I'm not sure how to handle this then.  Removing the containers UI and adding it back is also confusing.  When they navigate away from about:perferences, they will believe they are in the default context, but as soon as they hit enter on a url, they are in a different context.

So maybe we should just make about:preferences work in Container tabs.

Comment 18

[:Gijs (he/him)]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #17)
> So maybe we should just make about:preferences work in Container tabs.

OK. One thing we could do is show some kind of notification bar at the top of the page indicating that although about:preferences is open in a container tab, the settings will apply everywhere.

Comment 19

[Andrea Marchesini [:baku]]

Right. I changed my mind and I agree with Gijs. It's important to be consistent and if about:preferences works in privateBrowsing, it must work also in containers. My patch fixes the dialog/modal window thing. I still want to talk with smaug or another docShell peer to understand better why the target window has a non-system principal.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #13)
> Created attachment 8767958 [details] [diff] [review]
> docshell.patch
> 
> The issue here is that the comparison of docShell originAttributes fails.
> the accessing DS uses a system principal and UCI==1, but the target doesn't
> have a system principal and UCI==0.
First question I have is *why* does it have UCI==0? Is the implementation opening the tab modal window buggy? Does it not propagate the UCI properly?

And second, if the target doesn't have system principal, why does the patch help?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8767958 [details] [diff] [review]
docshell.patch

So, can't make this kind of changes without knowing exactly what is happening and where.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: docshell.patch

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8768866 [details] [diff] [review]
docshell.patch

Trying to build-up my experience reviewing docshell stuff, so I hope you don't mind that I feedback? myself.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: docshell.patch

Mike, smaug, here a check of the same type of docShell before inheriting OriginAttributes.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768881 [details] [diff] [review]
docshell.patch

>+  DocShellOriginAttributes attrs =
>+    nsDocShell::Cast(docShell)->GetOriginAttributes();
Ok, this is bringing back the docshell inheritance which was changed in bug 1227861, I think somewhat accidentally.


>--- a/embedding/components/windowwatcher/nsWindowWatcher.cpp
>+++ b/embedding/components/windowwatcher/nsWindowWatcher.cpp
>@@ -471,16 +471,22 @@ CheckUserContextCompatibility(nsIDocShel

Not about this bug, but I think special casing DEFAULT_USER_CONTEXT_ID in this method is a mistake and should be removed.
Please file a bug about it

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9a54d23bbc
DocShell child should inherit OriginAttributes from the parent DocShell, r=smaug

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8768881 [details] [diff] [review]
docshell.patch

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

::: dom/base/nsFrameLoader.cpp
@@ +2092,5 @@
>      webNav->SetSessionHistory(sessionHistory);
>    }
>  
>    DocShellOriginAttributes attrs;
> +  if (docShell->ItemType() == mDocShell->ItemType()) {

Note that docShell->ItemType() is already aliased in this scope as "parentType".

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

It is but this is actually easier to read, and this isn't performance critical code, so it was ok to me :)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df9a54d23bbc