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

Status: VERIFIED FIXED

Description

[Paul Theriault [:pauljt]]

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.

Comment 1

[Tanvi Vyas - out on leave [:tanvi]]

We need to determine the priority of this bug.

Comment 2

[Tanvi Vyas - out on leave [:tanvi]]

Can you test to see what context the tab that opens up in is?

Comment 3

[Tanvi Vyas - out on leave [:tanvi]]

Also adding Kamil.  Kamil, can you help test this on other platforms?

Comment 4

[Kamil Jozwiak [:kjozwiak]]

(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/

Comment 5

[Kamil Jozwiak [: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

Comment 6

[Andrea Marchesini [:baku]]

Created attachment 8754772 [details] [diff] [review]
no_window.patch

Comment 7

[Olli Pettay [:smaug]]

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.

Comment 8

[Andrea Marchesini [:baku]]

Created attachment 8755455 [details] [diff] [review]
no_window.patch

Comment 9

[Kamil Jozwiak [:kjozwiak]]

Baku, let me know if you still need feedback from me.

Comment 10

[Mike Conley (:mconley)]

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.

Comment 11

[Andrea Marchesini [:baku]]

Mike, I'll ping you on IRC today to talk about this issue. Together we can find a good solution.

Comment 12

[Andrea Marchesini [:baku]]

Created attachment 8757995 [details] [diff] [review]
no_window.patch

Comment 13

[Andrea Marchesini [:baku]]

Created attachment 8758045 [details] [diff] [review]
patch - version 2

Comment 14

[Olli Pettay [:smaug]]

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.

Comment 15

[Olli Pettay [:smaug]]

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.

Comment 16

[Kamil Jozwiak [:kjozwiak]]

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

Comment 17

[Andrea Marchesini [:baku]]

Thanks! This second patch should fix this issue.

Comment 18

[Andrea Marchesini [:baku]]

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.

Comment 19

[Andrea Marchesini [:baku]]

Created attachment 8758218 [details] [diff] [review]
no_window.patch

Comment 20

[Andrea Marchesini [:baku]]

Created attachment 8758228 [details] [diff] [review]
no_window.patch

Comment 21

[Andrea Marchesini [:baku]]

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

Removing the review request: I'm still not happy with this patch.

Comment 22

[Kamil Jozwiak [:kjozwiak]]

(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?

Comment 23

[Andrea Marchesini [:baku]]

No, please, don't test it. I want to improve this patch.

Comment 24

[Olli Pettay [:smaug]]

btw, I think we want to explicitly throw an exception (return nsresult error value) if one tries to re-set origin attributes.

Comment 25

[Andrea Marchesini [:baku]]

Created attachment 8758750 [details] [diff] [review]
no_window.patch

Comment 26

[Olli Pettay [:smaug]]

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.

Comment 27

[Andrea Marchesini [:baku]]

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.

Comment 28

[Olli Pettay [:smaug]]

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.)

Comment 29

[Andrea Marchesini [:baku]]

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.

Comment 30

[Olli Pettay [:smaug]]

(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]?

Comment 31

[Olli Pettay [:smaug]]

or does the requirement to be js implementable prevent use of [implicit_jscontext]? Then fine for a separate method.

Comment 32

[Andrea Marchesini [:baku]]

(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 33

[Olli Pettay [:smaug]]

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-

Comment 34

[Olli Pettay [:smaug]]

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.

Comment 35

[Andrea Marchesini [:baku]]

Created attachment 8759634 [details] [diff] [review]
no_window.patch

Comment 36

[Andrea Marchesini [:baku]]

Created attachment 8759635 [details] [diff] [review]
id.patch

Interdiff

Comment 37

[Olli Pettay [:smaug]]

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.

Comment 38

[Andrea Marchesini [:baku]]

Created attachment 8760315 [details] [diff] [review]
interdiff

Comment 39

[Olli Pettay [:smaug]]

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'.

Comment 40

[Andrea Marchesini [:baku]]

Created attachment 8760360 [details] [diff] [review]
interdiff

New interdiff.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d16086e85e
Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug

Comment 42

[Phil Ringnalda (:philor)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/900bc410537b
Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/900bc410537b

Comment 45

[Paul Theriault [:pauljt]]

This sounds like needs to be uplifted.

Comment 46

[Paul Theriault [:pauljt]]

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

Comment 47

[Liz Henry (:lizzard) (needinfo? me)]

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?

Comment 48

[Carsten Book [:Tomcat]]

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')

Comment 49

[Andrea Marchesini [:baku]]

Created attachment 8771778 [details] [diff] [review]
m-a

Comment 50

[Paul Theriault [:pauljt]]

(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.

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0622a239bad

Comment 52

[Kamil Jozwiak [:kjozwiak]]

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

Comment 53

[Kamil Jozwiak [:kjozwiak]]

I'll go through fx50 verification once the regression from bug # 1289571 has been fixed/addressed.

Comment 54

[Kamil Jozwiak [:kjozwiak]]

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.