Closed
Bug 1271516
Opened 5 years ago
Closed 5 years ago
File -> New Container Tab doesn't work if there are no windows open
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
VERIFIED
FIXED
mozilla50
People
(Reporter: pauljt, Assigned: baku)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [userContextId][domsecurity-active][uplift49+])
Attachments
(3 files, 11 obsolete files)
18.43 KB,
patch
|
Details | Diff | Splinter Review | |
6.81 KB,
patch
|
smaug
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
25.52 KB,
patch
|
Details | Diff | Splinter Review |
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 2•5 years ago
|
||
Can you test to see what context the tab that opens up in is?
Comment 3•5 years ago
|
||
Also adding Kamil. Kamil, can you help test this on other platforms?
Flags: needinfo?(kjozwiak)
Priority: -- → P1
Reporter | ||
Updated•5 years ago
|
QA Contact: ptheriault
Comment 4•5 years ago
|
||
(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)
Comment 5•5 years ago
|
||
(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
Updated•5 years ago
|
Assignee: nobody → amarchesini
Component: Security → DOM: Security
Updated•5 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [userContextId] → [userContextId][domsecurity-active]
Assignee | ||
Comment 6•5 years ago
|
||
Attachment #8754772 -
Flags: review?(bugs)
Attachment #8754772 -
Flags: feedback?(kjozwiak)
Comment 7•5 years ago
|
||
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•5 years ago
|
||
Attachment #8754772 -
Attachment is obsolete: true
Attachment #8754772 -
Flags: feedback?(kjozwiak)
Attachment #8755455 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•5 years ago
|
Attachment #8755455 -
Flags: review?(gijskruitbosch+bugs) → review?(mconley)
Comment 9•5 years ago
|
||
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-
Assignee | ||
Comment 11•5 years ago
|
||
Mike, I'll ping you on IRC today to talk about this issue. Together we can find a good solution.
Assignee | ||
Comment 12•5 years ago
|
||
Attachment #8755455 -
Attachment is obsolete: true
Attachment #8757995 -
Flags: review?(mconley)
Attachment #8757995 -
Flags: review?(bugs)
Assignee | ||
Comment 13•5 years ago
|
||
Comment 14•5 years ago
|
||
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-
Comment 15•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
Attachment #8757995 -
Attachment is obsolete: true
Attachment #8757995 -
Flags: review?(mconley)
Attachment #8757995 -
Flags: review-
Assignee | ||
Comment 17•5 years ago
|
||
Thanks! This second patch should fix this issue.
Flags: needinfo?(kjozwiak)
Assignee | ||
Updated•5 years ago
|
Attachment #8758045 -
Flags: review?(bugs)
Assignee | ||
Comment 18•5 years 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•5 years ago
|
||
Attachment #8758045 -
Attachment is obsolete: true
Attachment #8758045 -
Flags: review?(bugs)
Attachment #8758218 -
Flags: review?(bugs)
Assignee | ||
Comment 20•5 years ago
|
||
Attachment #8758218 -
Attachment is obsolete: true
Attachment #8758218 -
Flags: review?(bugs)
Attachment #8758228 -
Flags: review?(bugs)
Assignee | ||
Comment 21•5 years 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)
Comment 22•5 years ago
|
||
(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•5 years ago
|
||
No, please, don't test it. I want to improve this patch.
Flags: needinfo?(amarchesini)
Comment 24•5 years ago
|
||
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•5 years ago
|
||
Attachment #8758228 -
Attachment is obsolete: true
Attachment #8758750 -
Flags: review?(bugs)
Comment 26•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
Attachment #8759091 -
Flags: review?(bugs)
Comment 28•5 years ago
|
||
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.)
Assignee | ||
Comment 29•5 years ago
|
||
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)
Comment 30•5 years ago
|
||
(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•5 years ago
|
||
or does the requirement to be js implementable prevent use of [implicit_jscontext]? Then fine for a separate method.
Assignee | ||
Comment 32•5 years 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 33•5 years ago
|
||
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-
Comment 34•5 years ago
|
||
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•5 years ago
|
||
Attachment #8759543 -
Attachment is obsolete: true
Comment 37•5 years ago
|
||
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•5 years ago
|
||
Attachment #8759635 -
Attachment is obsolete: true
Attachment #8760315 -
Flags: review?(bugs)
Comment 39•5 years ago
|
||
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•5 years ago
|
||
New interdiff.
Attachment #8760315 -
Attachment is obsolete: true
Attachment #8760360 -
Flags: review?(bugs)
Updated•5 years ago
|
Attachment #8760360 -
Flags: review?(bugs) → review+
Comment 41•5 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/32d16086e85e Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug
Comment 42•5 years ago
|
||
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•5 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/900bc410537b Introducing nsIWebNavigation.setOriginAttributesBeforeLoading, r=smaug
Comment 44•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/900bc410537b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Reporter | ||
Comment 45•5 years ago
|
||
This sounds like needs to be uplifted.
Whiteboard: [userContextId][domsecurity-active] → [userContextId][domsecurity-active][uplift49+]
Reporter | ||
Comment 46•5 years 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
Comment 48•5 years ago
|
||
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•5 years ago
|
||
Reporter | ||
Comment 50•5 years 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•5 years ago
|
Attachment #8771778 -
Attachment is patch: true
Comment 51•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0622a239bad
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(amarchesini)
Comment 52•5 years ago
|
||
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•5 years ago
|
||
I'll go through fx50 verification once the regression from bug # 1289571 has been fixed/addressed.
Comment 54•5 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•