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

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kjozwiak, Assigned: baku)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla50
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [userContextId][domsecurity-active][uplift49-])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8762177 [details]
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
(Reporter)

Comment 1

2 years ago
Created attachment 8762178 [details]
example2.png
Priority: -- → P2
(Reporter)

Comment 2

2 years ago
Created attachment 8762184 [details]
correctExample.png

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

Comment 3

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → amarchesini
(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.
(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.
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-active]
Priority: P2 → P1
(Assignee)

Comment 6

a year ago
Created attachment 8767714 [details] [diff] [review]
part 1 - Opening about:preferences from the menu should not use a container tab.
Attachment #8767714 - Flags: review?(mconley)
(Assignee)

Comment 7

a year ago
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.
Flags: needinfo?(bram)
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
Attachment #8767714 - Flags: review?(mconley) → review+
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?
Attachment #8767714 - Flags: review+ → review-
(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).
(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?
Flags: needinfo?(bram)
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).
(Assignee)

Comment 13

a year ago
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.
I don't know if this is the right fix.
Attachment #8767958 - Flags: review?(bugs)
(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...
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.
(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...
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.
(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.
(Assignee)

Comment 19

a year ago
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.
Status: NEW → ASSIGNED
(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 on attachment 8767958 [details] [diff] [review]
docshell.patch

So, can't make this kind of changes without knowing exactly what is happening and where.
Attachment #8767958 - Flags: review?(bugs) → review-
(Assignee)

Comment 22

a year ago
Created attachment 8768866 [details] [diff] [review]
docshell.patch
Attachment #8767958 - Attachment is obsolete: true
Attachment #8768866 - Flags: review?(bugs)
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.
Attachment #8768866 - Flags: feedback?(mconley)
(Assignee)

Comment 24

a year ago
Created attachment 8768881 [details] [diff] [review]
docshell.patch

Mike, smaug, here a check of the same type of docShell before inheriting OriginAttributes.
Attachment #8768866 - Attachment is obsolete: true
Attachment #8768866 - Flags: review?(bugs)
Attachment #8768866 - Flags: feedback?(mconley)
Attachment #8768881 - Flags: review?(bugs)
Attachment #8768881 - Flags: feedback?(mconley)
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
Attachment #8768881 - Flags: review?(bugs) → review+
(Assignee)

Updated

a year ago
Blocks: 1285326

Comment 26

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9a54d23bbc
DocShell child should inherit OriginAttributes from the parent DocShell, r=smaug
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".
Attachment #8768881 - Flags: feedback?(mconley) → feedback+
It is but this is actually easier to read, and this isn't performance critical code, so it was ok to me :)

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df9a54d23bbc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Whiteboard: [userContextId][domsecurity-active] → [userContextId][domsecurity-active][uplift49-]
Duplicate of this bug: 1252800
You need to log in before you can comment on or make changes to this bug.