The default bug view has changed. See this FAQ.

File -> New Container Tab doesn't work if there are no windows open

VERIFIED FIXED in Firefox 49

Status

()

Core
DOM: Security
P1
normal
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: pauljt, Assigned: baku)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 verified, firefox50 verified)

Details

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

Attachments

(3 attachments, 11 obsolete attachments)

18.43 KB, patch
Details | Diff | Splinter Review
6.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
m-a
25.52 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 months ago
On OSx 10.11.4 with nightly, opening a new containers tab from the file menu doesnt work if there are no windows/tabs currently open.

STR: 
1. Close all tabs
2. File -> New Container Tab -> work

ER: 
New window opens with a containers tab

AR: 
A new default window open with a tab in the default context [1]

[1] I haven't actually tested if it IS the default context but there is no visual indication, so Im assuming that its the default context. It might actually be the desired context, and just be missing the  UI.
We need to determine the priority of this bug.
Whiteboard: [userContextId]
Can you test to see what context the tab that opens up in is?
Also adding Kamil.  Kamil, can you help test this on other platforms?
Flags: needinfo?(kjozwiak)
Priority: -- → P1
(Reporter)

Updated

11 months ago
QA Contact: ptheriault
(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Also adding Kamil.  Kamil, can you help test this on other platforms?

I don't think this affects Win/Ubuntu. When you close all the tabs under Win/Ubuntu, FX will shutdown and won't leave behind the Menu Bar like it does on OSX. However, I'm not sure if all the distro's behave the same way Ubuntu does. I can download/install a few other popular distro's [1] and see how they behave but I'm pretty certain there's no way to do this on Win. Once you close all the tabs, Windows will shut down the firefox.exe process.

[1] https://distrowatch.com/
Flags: needinfo?(kjozwiak)
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Can you test to see what context the tab that opens up in is?

It looks like this is actually using the default container and not just a UI issue. With the manual test case, I logged into bugzilla within the work container and closed the tab. While FX was still running without any windows opened, I opened the same work container once again but this time around I wasn't logged into bugzilla anymore.

Tanvi, I'm not 100% sure if the debugging portion of this is correct though, I might be completely wrong :/

Manual STR:
==========

* open m-c with a completely clean profile (all history/cookies cleared)
* open the work container and login into bugzilla (also tried amazon, facebook)
* once logged in, close the work container on OSX (don't shutdown firefox, just close the tab)
* Select File -> New Container Tab -> Work and load bugzilla (you won't be logged in)

Debugging via xCode:
===================

I added a break point at BasePrincipal.cpp#42 [1] and started opening containers through the file menu. While opening the containers with a window opened, mUserContextId was always the correct value. However, opening the containers without a window always produced a value of mUserContextId=0.

Opening containers with a window opened:
----------------------------------------

mozilla::PrincipalOriginAttributes::InheritFromDocShellToDoc(mozilla::DocShellOriginAttributes const&, nsIURI const*) at /Users/kjozwiak/projects/m-c-containers/caps/BasePrincipal.cpp:42
- aAttrs = (const mozilla::DocShellOriginAttributes &)
-- mozilla::OriginAttributes
--- mozilla::dom::OriginAttributesDictionary
---- mUserContextId = 1 (opening the personal container)
---- mUserContextId = 2 (opening the work container)
---- mUserContextId = 3 (opening the banking container)
---- mUserContextId = 4 (opening the shopping container)

Opening containers without a window being opened:
-------------------------------------------------

mozilla::PrincipalOriginAttributes::InheritFromDocShellToDoc(mozilla::DocShellOriginAttributes const&, nsIURI const*) at /Users/kjozwiak/projects/m-c-containers/caps/BasePrincipal.cpp:42
- aAttrs = (const mozilla::DocShellOriginAttributes &)
-- mozilla::OriginAttributes
--- mozilla::dom::OriginAttributesDictionary
---- mUserContextId = 0 (opening the personal container)
---- mUserContextId = 0 (opening the work container)
---- mUserContextId = 0 (opening the banking container)
---- mUserContextId = 0 (opening the shopping container)

[1] https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp?case=true&from=BasePrincipal.cpp#42
Assignee: nobody → amarchesini
Component: Security → DOM: Security
Status: NEW → ASSIGNED
Whiteboard: [userContextId] → [userContextId][domsecurity-active]
(Assignee)

Comment 6

10 months ago
Created attachment 8754772 [details] [diff] [review]
no_window.patch
Attachment #8754772 - Flags: review?(bugs)
Attachment #8754772 - Flags: feedback?(kjozwiak)
Comment on attachment 8754772 [details] [diff] [review]
no_window.patch

This is browser/ code so someone else should probably review this... but, 
accessing docshell obviously doesn't work in e10s in _loadURIWithFlags.
Attachment #8754772 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

10 months ago
Created attachment 8755455 [details] [diff] [review]
no_window.patch
Attachment #8754772 - Attachment is obsolete: true
Attachment #8754772 - Flags: feedback?(kjozwiak)
Attachment #8755455 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

10 months ago
Attachment #8755455 - Flags: review?(gijskruitbosch+bugs) → review?(mconley)
Baku, let me know if you still need feedback from me.
Comment on attachment 8755455 [details] [diff] [review]
no_window.patch

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

Hey baku,

Unless I'm mistaken, the code you've added to browser.js is going to fail in the event that the selected browser is remote and the load URI is not going to result in a remoteness flip. Or am I missing something?

::: browser/base/content/browser.js
@@ +829,5 @@
>    }
>    try {
>      if (!mustChangeProcess) {
> +      if (params.userContextId) {
> +        let docShell = browser.webNavigation.getInterface(Ci.nsIWebNavigation)

This isn't going to work in the case where the browser is remote, as we have no access to the docShell here.
Attachment #8755455 - Flags: review?(mconley) → review-
Blocks: 1276412
(Assignee)

Comment 11

10 months ago
Mike, I'll ping you on IRC today to talk about this issue. Together we can find a good solution.
(Assignee)

Comment 12

10 months ago
Created attachment 8757995 [details] [diff] [review]
no_window.patch
Attachment #8755455 - Attachment is obsolete: true
Attachment #8757995 - Flags: review?(mconley)
Attachment #8757995 - Flags: review?(bugs)
(Assignee)

Comment 13

10 months ago
Created attachment 8758045 [details] [diff] [review]
patch - version 2
Comment on attachment 8757995 [details] [diff] [review]
no_window.patch

So I think this makes the API too error prone, since origin attributes should be set only once.
I think nsDocShell needs to have some check for this and SetOriginAttributes would then throw if it is called again but with different origin attributes than what has been already set. And nsDocShell::LoadURIWithOptions( would need to check the return value of SetOriginAttributes.
Attachment #8757995 - Flags: review?(bugs) → review-
The version 2 makes the setup a bit better, but I think we still need to be rather strict that origin attributes are set just once and not changed afterwards.
Baku, with the patch from comment #13 applied to the latest m-c, I'm able to open all containers via the file menu whenever all the windows are closed. However, I'm receiving a lot of crashes whenever I try loading a website right after closing about:blank or simply loading about:blank or about:preferences in either a container window/tab or a default window. This is happening in both e10s and non-e10s modes. Once I remove the patch from comment # 13, I can't reproduce the crashes anymore.

STR:

* with the patch applied from comment #13, launch m-c via debug mode
* attempt loading about:blank, about:preferences, about:config etc..

OR

* with the patch applied from comment #13, launch m-c via debug mode
* select "File -> New Container Tab -> Shopping"
* inside the container, load about:blank, about:config, about:preferences etc..

OR

* with the patch applied from comment #13, launch m-c via debug mode
* close the about:blank window/tab that originally opened when loading m-c
* select "File -> New Container Tab -> Shopping"
* in the new shopping container, load ebay.com
 
Stack:

Assertion failure: uriSpec.EqualsLiteral("about:blank"), at /Users/kjozwiak/projects/m-c-containers/docshell/base/nsDocShell.cpp:14179
#01: nsDocShell::SetOriginAttributes(mozilla::DocShellOriginAttributes const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5568f1d]
#02: nsDocShell::LoadURIWithOptions(char16_t const*, unsigned int, nsIURI*, unsigned int, nsIInputStream*, nsIInputStream*, nsIURI*, mozilla::DocShellOriginAttributes*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5568147]
#03: nsDocShell::LoadURIWithOptions(char16_t const*, unsigned int, nsIURI*, unsigned int, nsIInputStream*, nsIInputStream*, nsIURI*, JS::Handle<JS::Value>, JSContext*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5568c77]
#04: non-virtual thunk to nsDocShell::LoadURIWithOptions(char16_t const*, unsigned int, nsIURI*, unsigned int, nsIInputStream*, nsIInputStream*, nsIURI*, JS::Handle<JS::Value>, JSContext*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5568d29]
#05: NS_InvokeByIndex[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1e78e0]
#06: CallMethodHelper::Invoke()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x156c954]
#07: CallMethodHelper::Call()[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x156309b]
#08: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x15422ea]
#09: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x15442bf]
#10: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x769795d]
#11: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ccae]
#12: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#13: js::CallFromStack(JSContext*, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ce7d]
#14: Interpret(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7672241]
#15: js::RunScript(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7667982]
#16: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cd7b]
#17: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#18: js::CallFromStack(JSContext*, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ce7d]
#19: Interpret(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7672241]
#20: js::RunScript(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7667982]
#21: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cd7b]
#22: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#23: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x765b326]
#24: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x757bb90]
#25: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7565a77]
#26: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x756b5e6]
#27: js::proxy_Call(JSContext*, unsigned int, JS::Value*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x756cde3]
#28: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x769795d]
#29: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cb79]
#30: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#31: js::CallFromStack(JSContext*, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ce7d]
#32: Interpret(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7672241]
#33: js::RunScript(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7667982]
#34: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cd7b]
#35: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#36: js::CallFromStack(JSContext*, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ce7d]
#37: Interpret(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7672241]
#38: js::RunScript(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7667982]
#39: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cd7b]
#40: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#41: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x765b326]
#42: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x757bb90]
#43: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7565a77]
#44: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x756b5e6]
#45: js::proxy_Call(JSContext*, unsigned int, JS::Value*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x756cde3]
#46: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x769795d]
#47: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cb79]
#48: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#49: js::CallFromStack(JSContext*, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ce7d]
#50: Interpret(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7672241]
#51: js::RunScript(JSContext*, js::RunState&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7667982]
#52: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cd7b]
#53: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#54: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x765b326]
#55: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x757bb90]
#56: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7565a77]
#57: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x756b5e6]
#58: js::proxy_Call(JSContext*, unsigned int, JS::Value*)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x756cde3]
#59: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x769795d]
#60: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767cb79]
#61: InternalCall(JSContext*, js::AnyInvokeArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767d077]
#62: js::CallFromStack(JSContext*, JS::CallArgs const&)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x767ce7d]
#63: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)[/Users/kjozwiak/projects/m-c-containers/objdir-ff-debug/dist/Nightly
(Assignee)

Updated

10 months ago
Attachment #8757995 - Attachment is obsolete: true
Attachment #8757995 - Flags: review?(mconley)
Attachment #8757995 - Flags: review-
(Assignee)

Comment 17

10 months ago
Thanks! This second patch should fix this issue.
Flags: needinfo?(kjozwiak)
(Assignee)

Updated

10 months ago
Attachment #8758045 - Flags: review?(bugs)
(Assignee)

Comment 18

10 months ago
Smaug, I ask you to review this because we already have the check about not overwrite an OriginAttributes if the docShell is not 'empty'. I guess this is enough.
(Assignee)

Comment 19

10 months ago
Created attachment 8758218 [details] [diff] [review]
no_window.patch
Attachment #8758045 - Attachment is obsolete: true
Attachment #8758045 - Flags: review?(bugs)
Attachment #8758218 - Flags: review?(bugs)
(Assignee)

Comment 20

10 months ago
Created attachment 8758228 [details] [diff] [review]
no_window.patch
Attachment #8758218 - Attachment is obsolete: true
Attachment #8758218 - Flags: review?(bugs)
Attachment #8758228 - Flags: review?(bugs)
(Assignee)

Comment 21

10 months ago
Comment on attachment 8758228 [details] [diff] [review]
no_window.patch

Removing the review request: I'm still not happy with this patch.
Attachment #8758228 - Flags: review?(bugs)
(In reply to Andrea Marchesini (:baku) from comment #21)
> Comment on attachment 8758228 [details] [diff] [review]
> no_window.patch
> 
> Removing the review request: I'm still not happy with this patch.

Baku, I started testing this but would you like me to wait for another patch? Or do you still want some feedback on this patch?
Flags: needinfo?(kjozwiak) → needinfo?(amarchesini)
(Assignee)

Comment 23

10 months ago
No, please, don't test it. I want to improve this patch.
Flags: needinfo?(amarchesini)
btw, I think we want to explicitly throw an exception (return nsresult error value) if one tries to re-set origin attributes.
(Assignee)

Comment 25

10 months ago
Created attachment 8758750 [details] [diff] [review]
no_window.patch
Attachment #8758228 - Attachment is obsolete: true
Attachment #8758750 - Flags: review?(bugs)
Comment on attachment 8758750 [details] [diff] [review]
no_window.patch

>   handleEvent(aEvent) {
>     // When the window is created, we want to inform the tabbrowser about
>     // the userContextId in use in order to update the UI correctly.
>     // Just because we cannot change the userContextId from an active docShell,
>     // we don't need to check DOMContentLoaded again.
>     this.uninit();
>-    let userContextId = content.document.nodePrincipal.originAttributes.userContextId;
>+    let userContextId = docShell.getOriginAttributes().userContextId;
Why this change? We should have the right UCI already in content's initial about:blank

>+
>+NS_IMETHODIMP
>+nsDocShell::LoadURIWithOptions(const char16_t* aURI,
>+                               uint32_t aLoadFlags,
>+                               nsIURI* aReferringURI,
>+                               uint32_t aReferrerPolicy,
>+                               nsIInputStream* aPostStream,
>+                               nsIInputStream* aHeaderStream,
>+                               nsIURI* aBaseURI)
This one should have nsresult as return type, not NS_IMETHODIMP

>+void
>+nsDocShell::SetOriginAttributes(const OriginAttributesPattern& aAttrs)
>+{
>+  if (aAttrs.Matches(mOriginAttributes)) {
>+    return;
>+  }
>+
>+#ifdef DEBUG
>+  AssertForOriginAttributesSetter();
>+#endif
>+
>+  aAttrs.Set(mOriginAttributes);
>+}
I don't understand this method at all. It fills some pattern based on mOriginAttributes.
Why is the method called SetOriginAttributes?
... oh, now I read what the Set() does. It is on wrong object.
You aren't setting anything on pattern, but on mOriginAttributes.



>+
> NS_IMETHODIMP
> nsDocShell::SetOriginAttributes(JS::Handle<JS::Value> aOriginAttributes,
>                                 JSContext* aCx)
> {
>-  DocShellOriginAttributes attrs;
>+  OriginAttributesPattern attrs;
Why pattern here?

>   if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) {
>     return NS_ERROR_INVALID_ARG;
>   }
> 
>   SetOriginAttributes(attrs);
>   return NS_OK;
> }
I think this method should throw if we're overriding existing origin attributes value in any way.
Attachment #8758750 - Flags: review?(bugs) → review-
(Assignee)

Comment 27

10 months ago
Created attachment 8759091 [details] [diff] [review]
no_window.patch

The reason why I use the pattern, is because often we receive an incomplete OriginAttributes as JS::Value.
Attachment #8758750 - Attachment is obsolete: true
Attachment #8759091 - Flags: review?(bugs)
(Assignee)

Updated

10 months ago
Attachment #8759091 - Flags: review?(bugs)
As I said on IRC, I'm not happy to pass around incomplete origin attributes.
We should be able initialize docshell's origin attributes once for good before any load has been initiated. And I think that should be done via some different method call than loadURI*, since that one can be used also after other loads have been done.
(initial about:blank isn't really loaded, so it can be a special case, though would be good to get rid of even that special case and really force origin attributes to be in place right after docshell is created.)

Updated

10 months ago
See Also: → bug 1257455
(Assignee)

Comment 29

10 months ago
Created attachment 8759543 [details] [diff] [review]
no_window.patch

Another working patch. Here some comments:

1. I call the new method 'setOriginAttributesBeforeLoading' because nsIDocShell already has a setOriginAttributes and nsDocShell is a nsIDocShell and a nsIWebNavigation.

2. setOriginAttributesBeforeLoading seems to me a good name because we have to enforce the fact that this must be called before any loadURI().

3. no tests. because I don't know how to create a test to simulate this macosx window-less behavior.
Attachment #8759091 - Attachment is obsolete: true
Attachment #8759543 - Flags: review?(bugs)
(In reply to Andrea Marchesini (:baku) from comment #29)
> 1. I call the new method 'setOriginAttributesBeforeLoading' because
> nsIDocShell already has a setOriginAttributes and nsDocShell is a
> nsIDocShell and a nsIWebNavigation.
But is there a reason for separate method implementation?
Could it be called the same in both interfaces and then nsDocShell just implements it once?
Make the method in nsIWebNavigation have [implicit_jscontext]?
or does the requirement to be js implementable prevent use of [implicit_jscontext]? Then fine for a separate method.
(Assignee)

Comment 32

10 months ago
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #31)
> or does the requirement to be js implementable prevent use of
> [implicit_jscontext]? Then fine for a separate method.

Right. I cannot use [implicit_jscontext] because nsIWebNavigation is implemented in JS as well.
Comment on attachment 8759543 [details] [diff] [review]
no_window.patch

>       LoadInOtherProcess(browser, {
>         uri: uri,
>         flags: flags,
>         referrer: referrer ? referrer.spec : null,
>         referrerPolicy: referrerPolicy,
>         postData: postData,
>+        userContextId: params.userContextId,
Why do we unconditionally send uci, even if params doesn't have it.?

>-    let userContextId = content.document.nodePrincipal.originAttributes.userContextId;
>+    let userContextId = docShell.originAttributes.userContextId;
this could use a comment about content.document being possibly the initial about:blank here

> NS_IMETHODIMP
>+nsDocShell::SetOriginAttributesBeforeLoading(JS::Handle<JS::Value> aOriginAttributes)
>+{
>+  if (!aOriginAttributes.isObject()) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  AutoJSAPI jsapi;
>+  jsapi.Init(&aOriginAttributes.toObject());
>+  JSContext* cx = jsapi.cx();
>+  if (NS_WARN_IF(!cx)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  DocShellOriginAttributes attrs;
>+  if (!aOriginAttributes.isObject() || !attrs.Init(cx, aOriginAttributes)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  SetOriginAttributes(attrs);
>+  return NS_OK;
I really really thing we must throw here if we're trying to override existing origin attributes.
So don't return NS_OK if SetOriginAttributes(attrs); fails.
Unfortunately that is void returning method atm. Please change it. MOZ_ASSERTs aren't good enough there.
frontend development happens often using opt builds, and addons certainly are developed using opt builds.
We want the exception to JS if origin attributes shouldn't be changed.

Because of that, r-
Attachment #8759543 - Flags: review?(bugs) → review-
I think docshell should even have some explicit flag to say that origin attributes have been set once, and it would refuse to override already set origin attributes.
(Assignee)

Comment 35

10 months ago
Created attachment 8759634 [details] [diff] [review]
no_window.patch
Attachment #8759543 - Attachment is obsolete: true
(Assignee)

Comment 36

10 months ago
Created attachment 8759635 [details] [diff] [review]
id.patch

Interdiff
Attachment #8759635 - Flags: review?(bugs)
Comment on attachment 8759635 [details] [diff] [review]
id.patch


>+nsresult
> nsDocShell::SetOriginAttributes(const DocShellOriginAttributes& aAttrs)
> {
>-  MOZ_ASSERT(mChildList.Length() == 0);
>+  MOZ_ASSERT(mChildList.IsEmpty());
>+  if (!mChildList.IsEmpty()) {
>+    return NS_ERROR_FAILURE;
>+  }
> 
>   // TODO: Bug 1273058 - mContentViewer should be null when setting origin
>   // attributes.
>   if (mContentViewer) {
>     nsIDocument* doc = mContentViewer->GetDocument();
>     if (doc) {
>       nsIURI* uri = doc->GetDocumentURI();
>       MOZ_ASSERT(uri);
Just for completeness I'd prefer to return error value also here. As the name hints, GetDocumentURI may in theory return null.

>       if (uri) {
>         nsAutoCString uriSpec;
>         uri->GetSpec(uriSpec);
>         MOZ_ASSERT(uriSpec.EqualsLiteral("about:blank"));
>+        if (!uriSpec.EqualsLiteral("about:blank")) {
>+          return NS_ERROR_FAILURE;
>+        }
>       }
>     }
>   }
> 
>   mOriginAttributes = aAttrs;
So what guarantees we aren't overriding mOriginAttributes (in case an about:blank was loaded explicitly afterwards)?
I really think we should have the flag explicitly checking that we aren't overriding anything. But that can be a separate bug.
Attachment #8759635 - Flags: review?(bugs) → review+
(Assignee)

Comment 38

10 months ago
Created attachment 8760315 [details] [diff] [review]
interdiff
Attachment #8759635 - Attachment is obsolete: true
Attachment #8760315 - Flags: review?(bugs)
Comment on attachment 8760315 [details] [diff] [review]
interdiff

+  if (aPrincipal) {
+    MOZ_ASSERT(ChromeUtils::IsOriginAttributesEqual(
+      BasePrincipal::Cast(aPrincipal)->OriginAttributesRef(),
+      mOriginAttributes));
+  }
Oh, surprising if we can assert that. I would have expected that mOriginAttributes is occasionally uninitialized here. Make sure to run this through tryserver (debug builds)
As I've said before, we _really_ should have a flag in nsDocShell to tell whether origin attributes have been initialized and use that to assert that we don't
re-initialize etc. I'd like to see such flag to be added even in this bug, perhaps in a separate patch.

>+  // If this is a new window, we must set the userContextId from the
>+  // subjectPrincipal.
>+  if (windowIsNew && subjectPrincipal) {
>+    DocShellOriginAttributes attrs;
>+    attrs.InheritFromDocToChildDocShell(BasePrincipal::Cast(subjectPrincipal)->OriginAttributesRef());
>+
>+    auto* docShell = static_cast<nsDocShell*>(newDocShell.get());
>+    docShell->SetOriginAttributes(attrs);
>+  }
>+
You should move this to be inside the following
if (windowIsNew) {
and then you can drop windowIsNew &&  from your 'if'.
Attachment #8760315 - Flags: review?(bugs) → review+
(Assignee)

Comment 40

10 months ago
Created attachment 8760360 [details] [diff] [review]
interdiff

New interdiff.
Attachment #8760315 - Attachment is obsolete: true
Attachment #8760360 - Flags: review?(bugs)
Attachment #8760360 - Flags: review?(bugs) → review+

Comment 41

10 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d16086e85e
Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2276c13873a816ce553801098b8f091306f2747f for frequent assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29614225&repo=mozilla-inbound

Comment 43

10 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/900bc410537b
Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug

Comment 44

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/900bc410537b
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

10 months ago
Blocks: 1279143
Blocks: 1279568
(Reporter)

Comment 45

9 months ago
This sounds like needs to be uplifted.
Whiteboard: [userContextId][domsecurity-active] → [userContextId][domsecurity-active][uplift49+]
(Assignee)

Updated

9 months ago
Blocks: 1284985
(Reporter)

Comment 46

9 months ago
Comment on attachment 8760360 [details] [diff] [review]
interdiff

Approval Request Comment
[Feature/regressing bug #]: Testpilot for containers
[User impact if declined]: UI will not function in certain situation with containers test-pilot. This patch also more generally addresses the issue of origin attributes being propagated from parent to child, so may address other as yet undiscovered OA bugs. 
[Describe test coverage new/current, TreeHerder]:  Some Origin Attributes testing is covered in CAPS unit tests, more is in progress.
[Risks and why]:  This involves platform changes that cannot be reprecated by an addon.  In order to do a Test Pilot experiment for Containers in September / Firefox 49 release, we need this code in release.  Without it, testing is delayed until November and getting the capabilities of the feature to users is delayed even further.
[String/UUID change made/needed]: None
Attachment #8760360 - Flags: approval-mozilla-aurora?
Comment on attachment 8760360 [details] [diff] [review]
interdiff

Let's try this in aurora. Paul is there a way to pref this feature off in case the uplifts don't go well?
Attachment #8760360 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox49: --- → affected
don,t apply cleanly to aurora

grafting 349039:900bc410537b "Bug 1271516 - Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug"
merging browser/base/content/browser.js
merging browser/base/content/tab-content.js
merging browser/base/content/utilityOverlay.js
merging browser/components/sessionstore/ContentRestore.jsm
merging docshell/base/nsDocShell.cpp
merging docshell/base/nsDocShell.h
merging docshell/base/nsIWebNavigation.idl
merging docshell/shistory/nsSHistory.cpp
merging dom/base/ChromeUtils.cpp
merging dom/base/ChromeUtils.h
merging embedding/browser/nsWebBrowser.cpp
merging embedding/components/windowwatcher/nsWindowWatcher.cpp
merging toolkit/components/remotebrowserutils/RemoteWebNavigation.js
merging toolkit/content/browser-child.js
warning: conflicts while merging docshell/base/nsDocShell.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(amarchesini)
(Assignee)

Comment 49

8 months ago
Created attachment 8771778 [details] [diff] [review]
m-a
(Reporter)

Comment 50

8 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #47)
> Comment on attachment 8760360 [details] [diff] [review]
> interdiff
> 
> Let's try this in aurora. Paul is there a way to pref this feature off in
> case the uplifts don't go well?

The containers feature is only enabled in nightly. Containers is controlled by the 'privacy.userContext.enabled' preference.

Updated

8 months ago
Attachment #8771778 - Attachment is patch: true

Comment 51

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0622a239bad
status-firefox49: affected → fixed
(Assignee)

Updated

8 months ago
Flags: needinfo?(amarchesini)
Went through verification using the following build:

* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-26-00-40-06-mozilla-aurora/
** also used the following local debug mode: fx49.0a2, buildId: 20160726143956, changeset: ebd239b4cbfb tip

OSX 10.11.6 x64 -> PASSED

I went through the following test cases when there's currently no windows fx opened:
* opened several tabs via File -> New Tab
* opened several tabs via File -> New Window
* opened every container via File -> New Container Tab
* opened several PB sessions via File -> New Private Window
* opened several non-e10s windows via File -> New non-e10s Window
* opened a new window via right clicking on the FX Icon under the dock and selecting New Window
* opened a new PB session via right clicking on the FX Icon under the dock and selecting New Private Window
* opened a new window by simply clicking on the FX icon under the dock
status-firefox49: fixed → verified
I'll go through fx50 verification once the regression from bug # 1289571 has been fixed/addressed.
Went through verification using the following builds:

* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-31-03-02-03-mozilla-central/
** fx50.0a1, buildId: 20160731030203, changeset: e5859dfe0bcb
* https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-02-03-04-37-mozilla-central/
** fx51.0a1, buildId: 20160802030437, changeset: ffac2798999c
* also used the following local debug mode: fx51.0a1, buildId: 20160802120943, changeset: 6608e5864780 tip (using --enable-debug)

I went through all the test cases from comment#52 without any issues.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.