Closed Bug 1303196 Opened 8 years ago Closed 8 years ago

Track which TabGroup and DocGroups a given window is in

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: nika, Assigned: nika)

References

(Blocks 4 open bugs)

Details

(Keywords: addon-compat)

Attachments

(7 files, 16 obsolete files)

Part 1: Add the DocGroup and TabGroup Objects
(deleted), patch
Details | Diff | Splinter Review
Part 4: Merge mFrameLoader and mOpener into mFrameLoaderOrOpener
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
Part 5: Remove a window namespacing test due to window namespacing behavior changes
(deleted), patch
nika
: review+
Details | Diff | Splinter Review
Part 3: Update the named window resolution logic to be scoped to TabGroup instead of being process-global
(deleted), patch
Details | Diff | Splinter Review
Part 6: Correctly set the opener when creating new tabs on android
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
Part 2: Connect the DocGroup and TabGroup objects to nsGlobalWindow and nsDocument, ensuring that Opener is set early enough that it is correct
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
Part 7: Updates to apply correctly after noopener changes
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review 
DocGroup and TabGroup replace Constellations from bug 1302275 as the mechanism for tracking windows which can access each-other synchronously and asynchronously respectively.
Attached patch Add TabGroup and DocGroup objects (obsolete) (deleted) — — Splinter Review 
This is my first pass at the new object based API. I tried to keep it simple while being usable.
Attachment #8791804 - Flags: review?(wmccloskey)
I should note that I'm not happy with the current state for iterating over the DocGroups in a TabGroup, but I wasn't sure what the best way to improve that situation was short of writing my own iterator class (which I _could_ do, but I would rather not).
Comment on attachment 8791804 [details] [diff] [review]
Add TabGroup and DocGroup objects

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

This looks good to me.

::: dom/base/DocGroup.cpp
@@ +4,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +void

/* static */ void
Also, any reason not to return an nsCString?

@@ +9,5 @@
> +DocGroup::GetKey(nsIPrincipal* aPrincipal, nsACString& aKey)
> +{
> +  aKey.Truncate();
> +  nsCOMPtr<nsIURI> uri;
> +  // XXX: Handle NullPrincipal, SystemPrincipal and co.

Don't we just want to return an empty string in this case? Technically SystemPrincipal is a big problem since it could access cross-origin iframes. But I don't think that will ever happen in the content process. Maybe we could assert that.

@@ +42,5 @@
> +  MOZ_ASSERT(mWindows.IsEmpty());
> +  mTabGroup->mDocGroups.RemoveEntry(mKey);
> +}
> +
> +TabGroup::~TabGroup() {

Brace goes on its own line.

::: dom/base/DocGroup.h
@@ +18,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +// Two browsing contexts are considered "related" if they are reachable from
> +// one anther through window.opener, window.parent, or window.frames.

another

@@ +24,5 @@
> +// Two browsing contexts are considered "similar-origin" if they can be made
> +// to have the same origin by setting document.domain.
> +//
> +// A TabGroup contains all "related" BrowsingContexts, grouped into
> +// "similar-origin" DocGroups.

This is a little wrong, because a browsing context is basically an outer window, and DocGroups contain inner windows. How about this?

```A TabGroup is a set of browsing contexts that are all "related". Within a TabGroup, browsing contexts are broken up into "similar-origin" DocGroups. In more detail, a DocGroup is actually a collection of inner windows, and a TabGroup is a collection of DocGroups. A TabGroup typically will contain (through its DocGroups) the inner windows from one or more tabs related by window.opener. A DocGroup is a member of exactly one TabGroup. Inner windows that aren't the current inner window of an outer window are not part of any DocGroup.```

@@ +26,5 @@
> +//
> +// A TabGroup contains all "related" BrowsingContexts, grouped into
> +// "similar-origin" DocGroups.
> +//
> +// A frame script may synchronously access only a single TabGroup, while content

Please take out the part about frame scripts since it's not really true due to JSMs.

@@ +71,5 @@
> +class TabGroup {
> +private:
> +  class HashEntry : public nsCStringHashKey {
> +  public:
> +    DocGroup* mDocGroup;

Please comment the fact that this pointer is weak. The DocGroup destructor removes itself from the TabGroup.

@@ +72,5 @@
> +private:
> +  class HashEntry : public nsCStringHashKey {
> +  public:
> +    DocGroup* mDocGroup;
> +    explicit HashEntry(const nsACString& aKey);

Just wondering if you need this variant. nsCStringHashKey doesn't seem to have it.

@@ +86,5 @@
> +
> +  TabGroup() {}
> +
> +  already_AddRefed<DocGroup>
> +  GetDocGroup(const nsACString& aKey);

Seems like this should be private. Please comment that it returns null if the given key hasn't been seen yet.

::: dom/base/nsGlobalWindow.cpp
@@ +14413,5 @@
> +#ifdef DEBUG
> +  // Sanity check that our tabgroup matches our opener or parent
> +  RefPtr<nsGlobalWindow> top = GetTopInternal();
> +  RefPtr<nsPIDOMWindowOuter> opener = GetOpener();
> +  MOZ_ASSERT(!top || (top->mTabGroup == mTabGroup));

MOZ_ASSERT_IF

@@ +14414,5 @@
> +  // Sanity check that our tabgroup matches our opener or parent
> +  RefPtr<nsGlobalWindow> top = GetTopInternal();
> +  RefPtr<nsPIDOMWindowOuter> opener = GetOpener();
> +  MOZ_ASSERT(!top || (top->mTabGroup == mTabGroup));
> +  MOZ_ASSERT(!opener || (Cast(opener)->mTabGroup == mTabGroup));

MOZ_ASSERT_IF

@@ +14452,2 @@
>      }
> +    MOZ_ASSERT(false, "The docgroup of an inner window should not change");

Let's make this a MOZ_CRASH so it hits in release builds.

@@ +14462,5 @@
> +{
> +  MOZ_RELEASE_ASSERT(IsOuterWindow());
> +  nsGlobalWindow* inner = GetCurrentInnerWindowInternal();
> +  if (inner && inner->mDocGroup) {
> +    // Leave our DocGroup if we are in one

Do we need this code? If we take it out, SwitchDocGroup will assert a little later in this case. That seems more correct to me. Did you find cases where we're already in a document group when we call InheritTabGroupFrom? Maybe I'm misunderstanding the initialization order.
Attachment #8791804 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #3)
> ::: dom/base/DocGroup.cpp
> @@ +4,5 @@
> > +
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +void
> 
> /* static */ void
> Also, any reason not to return an nsCString?

If the caller uses nsAutoCString this saves an allocation. *shrug*

> @@ +9,5 @@
> > +DocGroup::GetKey(nsIPrincipal* aPrincipal, nsACString& aKey)
> > +{
> > +  aKey.Truncate();
> > +  nsCOMPtr<nsIURI> uri;
> > +  // XXX: Handle NullPrincipal, SystemPrincipal and co.
> 
> Don't we just want to return an empty string in this case? Technically
> SystemPrincipal is a big problem since it could access cross-origin iframes.
> But I don't think that will ever happen in the content process. Maybe we
> could assert that.

I think for the null principal we want to put every null principal in its own DocGroup. With the system principal, I think that "" works, yes.

> @@ +14462,5 @@
> > +{
> > +  MOZ_RELEASE_ASSERT(IsOuterWindow());
> > +  nsGlobalWindow* inner = GetCurrentInnerWindowInternal();
> > +  if (inner && inner->mDocGroup) {
> > +    // Leave our DocGroup if we are in one
> 
> Do we need this code? If we take it out, SwitchDocGroup will assert a little
> later in this case. That seems more correct to me. Did you find cases where
> we're already in a document group when we call InheritTabGroupFrom? Maybe
> I'm misunderstanding the initialization order.

I can try a build which MOZ_RELEASE_ASSERTs that there is no inner window in the outer window when it is given an outer window to inherit its tab group from on try.
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0be7d1b7a9a
Add TabGroup and DocGroup objects, r=billm
Backed out for asserting in devtools test: top->mTabGroup == mTabGroup, at dom/base/nsGlobalWindow.cpp:14425:

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef020a0ba34

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b0be7d1b7a9acb28c6f2e4b0b580df805bbadb8b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=36459817&repo=mozilla-inbound

18:20:36     INFO -  132 INFO TEST-START | devtools/client/framework/test/browser_toolbox_hosts.js
...
18:20:42     INFO -  [2283] WARNING: NS_ENSURE_TRUE(!mParent || mParent == docLoaderService) failed: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/docshell/base/nsDocShell.cpp, line 3251
18:20:42     INFO -  [2283] WARNING: NS_ENSURE_TRUE(!mParent || mParent == docLoaderService) failed: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/docshell/base/nsDocShell.cpp, line 3251
18:20:42     INFO -  [GFX3-]: Surface width or height <= 0!
18:20:42     INFO -  [GFX3-]: Surface width or height <= 0!
18:20:42     INFO -  [GFX3-]: Surface width or height <= 0!
18:20:42     INFO -  [GFX3-]: Surface width or height <= 0!
18:20:42     INFO -  --DOCSHELL 0x971a4800 == 15 [pid = 2283] [id = 110]
18:20:42     INFO -  [GFX3-]: Surface width or height <= 0!
18:20:42     INFO -  [GFX3-]: Surface width or height <= 0!
18:20:42     INFO -  ++DOCSHELL 0x876e8c00 == 16 [pid = 2283] [id = 111]
18:20:42     INFO -  ++DOMWINDOW == 56 (0x96debc00) [pid = 2283] [serial = 250] [outer = (nil)]
18:20:42     INFO -  ++DOMWINDOW == 57 (0x973c1800) [pid = 2283] [serial = 251] [outer = 0x96debc00]
18:20:42     INFO -  [2283] WARNING: NS_ENSURE_TRUE(aURI) failed: file /builds/slave/m-in-lx-d-00000000000000000000/build/src/netwerk/dns/nsEffectiveTLDService.cpp, line 177
18:20:42     INFO -  Assertion failure: top->mTabGroup == mTabGroup, at /builds/slave/m-in-lx-d-00000000000000000000/build/src/dom/base/nsGlobalWindow.cpp:14425
18:21:11     INFO -  #01: nsGlobalWindow::GetTabGroup [dom/base/nsGlobalWindow.cpp:14425]
18:21:11     INFO -  #02: nsGlobalWindow::GetTabGroup [dom/base/nsGlobalWindow.cpp:14419]
18:21:11     INFO -  #03: nsGlobalWindow::SwitchDocGroup [dom/base/nsGlobalWindow.cpp:14463]
18:21:11     INFO -  #04: nsGlobalWindow::InnerSetNewDocument [dom/base/nsGlobalWindow.cpp:3022]
18:21:11     INFO -  #05: nsGlobalWindow::SetNewDocument [js/public/RootingAPI.h:700]
18:21:11     INFO -  #06: nsDocumentViewer::InitInternal [layout/base/nsDocumentViewer.cpp:876]
18:21:11     INFO -  #07: nsDocumentViewer::Init [layout/base/nsDocumentViewer.cpp:619]
18:21:11     INFO -  #08: nsDocShell::SetupNewViewer [docshell/base/nsDocShell.cpp:9383]
18:21:11     INFO -  #09: nsDocShell::Embed [docshell/base/nsDocShell.cpp:7237]
18:21:11     INFO -  #10: nsDocShell::CreateAboutBlankContentViewer [docshell/base/nsDocShell.cpp:8097]
18:21:11     INFO -  #11: nsDocShell::EnsureContentViewer [docshell/base/nsDocShell.cpp:7967]
18:21:11     INFO -  #12: nsDocShell::GetInterface [docshell/base/nsDocShell.cpp:969]
18:21:11     INFO -  #13: nsGetInterface::operator() [xpcom/glue/nsIInterfaceRequestorUtils.cpp:18]
18:21:11     INFO -  #14: nsDocLoader::DocLoaderIsEmpty [xpcom/glue/nsCOMPtr.h:1166]
18:21:11     INFO -  #15: nsDocLoader::OnStopRequest [uriloader/base/nsDocLoader.cpp:612]
18:21:11     INFO -  #16: mozilla::net::nsLoadGroup::RemoveRequest [netwerk/base/nsLoadGroup.cpp:635]
18:21:11     INFO -  #17: mozilla::net::nsLoadGroup::Cancel [netwerk/base/nsLoadGroup.cpp:273]
18:21:11     INFO -  #18: nsDocLoader::Stop [uriloader/base/nsDocLoader.cpp:243]
18:21:11     INFO -  #19: nsDocShell::Stop [docshell/base/nsDocShell.cpp:5514]
18:21:11     INFO -  #20: nsDocShell::InternalLoad [docshell/base/nsDocShell.cpp:10508]
18:21:11     INFO -  #21: nsDocShell::LoadURI [docshell/base/nsDocShell.cpp:1561]
18:21:11     INFO -  #22: nsFrameLoader::ReallyStartLoadingInternal [dom/base/nsFrameLoader.cpp:493]
18:21:11     INFO -  #23: nsFrameLoader::ReallyStartLoading [dom/base/nsFrameLoader.cpp:377]
18:21:11     INFO -  #24: nsDocument::MaybeInitializeFinalizeFrameLoaders [dom/base/nsDocument.cpp:6921]
18:21:11     INFO -  #25: mozilla::detail::RunnableMethodArguments<>::apply<nsDocument, void (nsDocument::*)()> [xpcom/glue/nsThreadUtils.h:729]
18:21:11     INFO -  #26: mozilla::detail::RunnableMethodImpl<void (nsDocument::*)(), true, false>::Run [xpcom/glue/nsThreadUtils.h:764]
18:21:11     INFO -  #27: nsContentUtils::RemoveScriptBlocker [xpcom/glue/nsCOMPtr.h:1088]
18:21:11     INFO -  #28: nsAutoScriptBlocker::~nsAutoScriptBlocker [dom/base/nsContentUtils.h:2856]
18:21:11     INFO -  #29: mozilla::dom::Element::SetAttr [dom/base/Element.cpp:2370]
18:21:11     INFO -  #30: mozilla::dom::Element::SetAttr [dom/base/Element.h:486]
18:21:11     INFO -  #31: mozilla::dom::Element::SetAttribute [dom/bindings/ErrorResult.h:317]
18:21:11     INFO -  #32: mozilla::dom::ElementBinding::setAttribute [obj-firefox/dom/bindings/ElementBinding.cpp:725]
18:21:11     INFO -  #33: mozilla::dom::GenericBindingMethod [dom/bindings/BindingUtils.cpp:2814]
18:21:11     INFO -  #34: js::CallJSNative [js/src/jscntxtinlines.h:240]
18:21:11     INFO -  #35: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:458]
18:21:11     INFO -  #36: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #37: js::Call [js/src/vm/Interpreter.cpp:522]
18:21:11     INFO -  #38: js::Wrapper::call [js/public/RootingAPI.h:706]
18:21:11     INFO -  #39: js::CrossCompartmentWrapper::call [js/src/proxy/CrossCompartmentWrapper.cpp:333]
18:21:11     INFO -  #40: xpc::AddonWrapper<js::CrossCompartmentWrapper>::call [js/xpconnect/wrappers/AddonWrapper.cpp:141]
18:21:11     INFO -  #41: js::Proxy::call [js/src/proxy/Proxy.cpp:401]
18:21:11     INFO -  #42: js::proxy_Call [js/src/proxy/Proxy.cpp:690]
18:21:11     INFO -  #43: js::CallJSNative [js/src/jscntxtinlines.h:240]
18:21:11     INFO -  #44: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:446]
18:21:11     INFO -  #45: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #46: Interpret [js/src/vm/Interpreter.cpp:2922]
18:21:11     INFO -  #47: js::RunScript [js/src/vm/Interpreter.cpp:404]
18:21:11     INFO -  #48: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:476]
18:21:11     INFO -  #49: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #50: js::Call [js/src/vm/Interpreter.cpp:522]
18:21:11     INFO -  #51: js::fun_call [js/public/RootingAPI.h:706]
18:21:11     INFO -  #52: js::CallJSNative [js/src/jscntxtinlines.h:240]
18:21:11     INFO -  #53: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:458]
18:21:11     INFO -  #54: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #55: js::Call [js/src/vm/Interpreter.cpp:522]
18:21:11     INFO -  #56: js::Wrapper::call [js/public/RootingAPI.h:706]
18:21:11     INFO -  #57: js::CrossCompartmentWrapper::call [js/src/proxy/CrossCompartmentWrapper.cpp:333]
18:21:11     INFO -  #58: js::Proxy::call [js/src/proxy/Proxy.cpp:401]
18:21:11     INFO -  #59: js::proxy_Call [js/src/proxy/Proxy.cpp:690]
18:21:11     INFO -  #60: js::CallJSNative [js/src/jscntxtinlines.h:240]
18:21:11     INFO -  #61: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:446]
18:21:11     INFO -  #62: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #63: js::jit::DoCallFallback [js/src/jit/BaselineIC.cpp:6010]
18:21:11     INFO -  #64: ??? (???:???)
18:21:11     INFO -  #65: ??? (???:???)
18:21:11     INFO -  #66: ??? (???:???)
18:21:11     INFO -  #67: ??? (???:???)
18:21:11     INFO -  #68: ??? (???:???)
18:21:11     INFO -  #69: ??? (???:???)
18:21:11     INFO -  #70: ??? (???:???)
18:21:11     INFO -  #71: EnterBaseline [js/src/jit/BaselineJIT.cpp:157]
18:21:11     INFO -  #72: js::jit::EnterBaselineMethod [js/src/jit/BaselineJIT.cpp:194]
18:21:11     INFO -  #73: js::RunScript [js/src/vm/Interpreter.cpp:395]
18:21:11     INFO -  #74: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:476]
18:21:11     INFO -  #75: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #76: js::Call [js/src/vm/Interpreter.cpp:522]
18:21:11     INFO -  #77: JS_CallFunctionValue [js/src/jsapi.cpp:2776]
18:21:11     INFO -  #78: nsXPCComponents_Utils::CallFunctionWithAsyncStack [js/xpconnect/src/XPCComponents.cpp:2711]
18:21:11     INFO -  #79: NS_InvokeByIndex
18:21:11     INFO -  #80: CallMethodHelper::Call [js/xpconnect/src/XPCWrappedNative.cpp:1382]
18:21:11     INFO -  #81: XPCWrappedNative::CallMethod [js/xpconnect/src/XPCWrappedNative.cpp:1349]
18:21:11     INFO -  #82: XPC_WN_CallMethod [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1143]
18:21:11     INFO -  #83: ??? (???:???)
18:21:11     INFO -  #84: ??? (???:???)
18:21:11     INFO -  #85: ??? (???:???)
18:21:11     INFO -  #86: EnterBaseline [js/src/jit/BaselineJIT.cpp:157]
18:21:11     INFO -  #87: js::jit::EnterBaselineMethod [js/src/jit/BaselineJIT.cpp:194]
18:21:11     INFO -  #88: js::RunScript [js/src/vm/Interpreter.cpp:395]
18:21:11     INFO -  #89: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:476]
18:21:11     INFO -  #90: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #91: js::Call [js/src/vm/Interpreter.cpp:522]
18:21:11     INFO -  #92: js::PromiseReactionJob [js/src/builtin/Promise.cpp:1104]
18:21:11     INFO -  #93: js::CallJSNative [js/src/jscntxtinlines.h:240]
18:21:11     INFO -  #94: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:458]
18:21:11     INFO -  #95: InternalCall [js/src/vm/Interpreter.cpp:504]
18:21:11     INFO -  #96: js::Call [js/src/vm/Interpreter.cpp:522]
18:21:11     INFO -  #97: JS::Call [js/src/jsapi.cpp:2836]
18:21:11     INFO -  #98: mozilla::dom::PromiseJobCallback::Call [obj-firefox/dom/bindings/PromiseBinding.cpp:62]
18:21:11     INFO -  #99: PromiseJobRunnable::Run [obj-firefox/dist/include/mozilla/dom/PromiseBinding.h:176]
18:21:11     INFO -  #100: mozilla::dom::Promise::PerformMicroTaskCheckpoint [dom/promise/Promise.cpp:1063]
18:21:11     INFO -  #101: mozilla::CycleCollectedJSContext::AfterProcessTask [xpcom/base/CycleCollectedJSContext.cpp:1395]
18:21:11     INFO -  #102: XPCJSContext::AfterProcessTask [js/xpconnect/src/XPCJSContext.cpp:3601]
18:21:11     INFO -  #103: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1098]
18:21:11     INFO -  #104: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:290]
18:21:11     INFO -  #105: mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:97]
18:21:11     INFO -  #106: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:232]
18:21:11     INFO -  #107: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:490]
18:21:11     INFO -  #108: nsBaseAppShell::Run [widget/nsBaseAppShell.cpp:158]
18:21:11     INFO -  #109: nsAppStartup::Run [toolkit/components/startup/nsAppStartup.cpp:284]
18:21:11     INFO -  #110: XREMain::XRE_mainRun [toolkit/xre/nsAppRunner.cpp:4420]
18:21:11     INFO -  #111: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4552]
18:21:11     INFO -  #112: XRE_main [toolkit/xre/nsAppRunner.cpp:4644]
18:21:11     INFO -  #113: do_main [browser/app/nsBrowserApp.cpp:281]
18:21:11     INFO -  #114: main [browser/app/nsBrowserApp.cpp:414]
Flags: needinfo?(michael)
Attached patch Add TabGroup and DocGroup objects (obsolete) (deleted) — — Splinter Review 
I had to make a bunch of changes to the way we handle chrome TabGroups (namely all chrome docshells are in the same singleton chrome tabgroup now) in order to avoid the assertion failures which caused this patch to be backed out.

Asking for a new review because of the relative scale of the changes.
Attachment #8795019 - Flags: review?(wmccloskey)
Attachment #8791804 - Attachment is obsolete: true
Flags: needinfo?(michael)
Comment on attachment 8795019 [details] [diff] [review]
Add TabGroup and DocGroup objects

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

::: dom/base/nsGlobalWindow.cpp
@@ +14486,5 @@
> +  // If we have an inner window, then that inner window's current doc group is
> +  // within our current tab group. As we are inheriting our tab group (and thus
> +  // changing away from the default of having a new tab group per window), we
> +  // need to remove that inner window from its doc group before we switch to the
> +  // new tab group.

Just to clarify a point from the previous patch, this means that SetNewDocument is being called before SetDocShell or SetOpenerWindow?

::: dom/base/nsGlobalWindow.h
@@ +1939,5 @@
>    nsAutoPtr<mozilla::dom::VREventObserver> mVREventObserver;
>  
> +  RefPtr<mozilla::dom::DocGroup> mDocGroup; // Inner window only
> +  RefPtr<mozilla::dom::TabGroup> mTabGroup; // Outer window only
> +  bool mIndependentTabGroup; // Outer window only. True if not inherited tab group

IIUC, this exists to assert that we only change mTabGroup once. A different name, like mTabGroupCanChange, might make that more clear.

It also might make sense to set this flag to false once someone else has inherited our TabGroup. I definitely think we want to prevent changes in that case.
Attachment #8795019 - Flags: review?(wmccloskey) → review+
Comment on attachment 8795019 [details] [diff] [review]
Add TabGroup and DocGroup objects

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

::: dom/base/nsGlobalWindow.h
@@ +1719,4 @@
>  public:
>  
> +  mozilla::dom::TabGroup* GetTabGroup();
> +  mozilla::dom::DocGroup* GetDocGroup();

Oh, one other style thing: these should just be TabGroup()/DocGroup() since they're infallible.
Comment on attachment 8795019 [details] [diff] [review]
Add TabGroup and DocGroup objects

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

::: dom/base/DocGroup.h
@@ +36,5 @@
> +// DocGroup.
> +
> +class TabGroup;
> +
> +class DocGroup {

And the brace should go on its own line in class definitions.

@@ +46,5 @@
> +  friend class TabGroup;
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DocGroup)
> +
> +  static void GetKey(nsIPrincipal* aPrincipal, nsACString& aString);
> +  bool MatchesKey(const nsACString& aKey) {

Braces in methods should go on their own line. For one-liners, you can do:
  void Foo() { bar(); }

@@ +72,5 @@
> +  WindowArray mWindows;
> +};
> +
> +
> +class TabGroup {

Same here.
Comment on attachment 8795019 [details] [diff] [review]
Add TabGroup and DocGroup objects

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

::: dom/base/DocGroup.h
@@ +64,5 @@
> +  }
> +
> +private:
> +  DocGroup(TabGroup* aTabGroup, const nsACString& aKey);
> +  ~DocGroup();

And these destructors should be virtual.
(In reply to Bill McCloskey (:billm) from comment #8)
> ::: dom/base/nsGlobalWindow.cpp
> @@ +14486,5 @@
> > +  // If we have an inner window, then that inner window's current doc group is
> > +  // within our current tab group. As we are inheriting our tab group (and thus
> > +  // changing away from the default of having a new tab group per window), we
> > +  // need to remove that inner window from its doc group before we switch to the
> > +  // new tab group.
> 
> Just to clarify a point from the previous patch, this means that
> SetNewDocument is being called before SetDocShell or SetOpenerWindow?

Yes. I tried to run firefox with this code removed and everything crashed and burned.

> ::: dom/base/nsGlobalWindow.h
> @@ +1939,5 @@
> >    nsAutoPtr<mozilla::dom::VREventObserver> mVREventObserver;
> >  
> > +  RefPtr<mozilla::dom::DocGroup> mDocGroup; // Inner window only
> > +  RefPtr<mozilla::dom::TabGroup> mTabGroup; // Outer window only
> > +  bool mIndependentTabGroup; // Outer window only. True if not inherited tab group
> 
> IIUC, this exists to assert that we only change mTabGroup once. A different
> name, like mTabGroupCanChange, might make that more clear.

Sounds good. Yes, it is there so that I can RELEASE_ASSERT and make sure people don't do bad things.

> It also might make sense to set this flag to false once someone else has
> inherited our TabGroup. I definitely think we want to prevent changes in
> that case.

We already do that, we set mTabGroupCanChange for both aWindow and this in InheritTabGroupFrom.

(In reply to Bill McCloskey (:billm) from comment #11)
> > +  ~DocGroup();
> 
> And these destructors should be virtual.

Wouldn't it be better to just mark TabGroup and DocGroup as final?

(In reply to Bill McCloskey (:billm) from comment #9)
> > +  mozilla::dom::TabGroup* GetTabGroup();
> > +  mozilla::dom::DocGroup* GetDocGroup();
> 
> Oh, one other style thing: these should just be TabGroup()/DocGroup() since
> they're infallible.

If I call them TabGroup() and DocGroup() then the names conflict in the .cpp with the type names. I called them GetTabGroup and GetDocGroup() instead because that way I avoid the name shadowing. If you really feel like they should be called TabGroup() and DocGroup() I can disambiguate all uses of the typename inside nsGlobalWindow.cpp, but it didn't feel like that big of a deal to me.
Attached patch Add TabGroup and DocGroup objects (obsolete) (deleted) — — Splinter Review 
Updated
Attachment #8795019 - Attachment is obsolete: true
Attached patch Part 1: Add the DocGroup and TabGroup Objects (obsolete) (deleted) — — Splinter Review 
I think this version of the patches finally will pass all of the tests. We'll see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b921da77ce8ae743df258cb3974e4c59209e1ab

This version is updated over the previous one a lot.
Attachment #8798991 - Flags: review?(wmccloskey)
Attachment #8795722 - Attachment is obsolete: true
Attachment #8798996 - Flags: review?(bzbarsky)
Comment on attachment 8798992 [details] [diff] [review]
Part 2: Connect the DocGroup and TabGroup objects to nsGlobalWindow and nsDocument, ensuring that Opener is set early enough that it is correct

>+mozilla::dom::DocGroup*
>+nsIDocument::DocGroup()
>+{
>+#ifdef DEBUG
>+  // Sanity check that we have an up-to-date and accurate docgroup
>+  if (mDocGroup) {
If you name the method DocGroup() and not GetDocGroup(), you're hinting to the caller the method never returns null.
So, you shouldn't need to have this if-check here.
But, looks like DocGroup() can actually return null, at least in case of data documents (document.implementation.createDocument("", ""))
So, rename the method to GetDocGroup() (or make it somehow infallible)
>+nsGlobalWindow::MaybeSetOpenerWindow(nsPIDOMWindowOuter* aOpener,
>+                                     bool aOriginalOpener)
>+{
>+  FORWARD_TO_OUTER_VOID(MaybeSetOpenerWindow, (aOpener, aOriginalOpener));
>+
>+  nsWeakPtr opener = do_GetWeakReference(aOpener);
>+  if (opener == mOpener) {
>+    return;
>+  }
>+
>+  SetOpenerWindow(aOpener, aOriginalOpener);
I don't understand this method. Couldn't you just merge this to SetOpenerWindow?
>+nsGlobalChromeWindow::SetInitialOpenerWindow(mozIDOMWindowProxy* aOpenerWindow)
>+{
>+  MOZ_ASSERT(!mInitialOpenerWindow);
>+  mInitialOpenerWindow = aOpenerWindow;
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsGlobalChromeWindow::TakeInitialOpenerWindow(mozIDOMWindowProxy** aOpenerWindow)
>+{
>+  // Intentionally forget our own member
>+  mInitialOpenerWindow.forget(aOpenerWindow);
>+  return NS_OK;
>+}
Why do we need this initial opener stuff?

>+mozilla::dom::DocGroup*
>+nsGlobalWindow::DocGroup()
>+{
>+  nsIDocument* doc = GetExtantDoc();
>+  if (doc) {
>+    return doc->DocGroup();
>   }
>-
>-  aConstellation.Assign(mConstellation);
>+  return nullptr;
> }
Since the method may return null, rename it GetDocGroup() so that callers know to null check the return value.
> public:
>+  nsPIDOMWindowOuter*
>+  SanitizeOpener(nsPIDOMWindowOuter* aOpener, bool aCallerIsChrome);
This method returns something, so should it be called 
GetSanitizedOpener or some such?

>+++ b/dom/base/nsIFrameLoader.idl
>@@ -13,16 +13,17 @@ interface nsIFrame;
> interface nsSubDocumentFrame;
> interface nsIMessageSender;
> interface nsIVariant;
> interface nsIDOMElement;
> interface nsITabParent;
> interface nsILoadContext;
> interface nsIPrintSettings;
> interface nsIWebProgressListener;
>+interface mozIDOMWindowProxy;
> 
> [scriptable, builtinclass, uuid(1645af04-1bc7-4363-8f2c-eb9679220ab1)]
> interface nsIFrameLoader : nsISupports
> {
>   /**
>    * Get the docshell from the frame loader.
>    */
>   readonly attribute nsIDocShell docShell;
I wonder why you need interface mozIDOMWindowProxy; here

>+nsGenericHTMLFrameElement::PresetOpenerWindow(mozIDOMWindowProxy* aWindow, ErrorResult& aRv)
>+{
>+  if (NS_WARN_IF(mFrameLoader)) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return;
>+  }
>+  mOpenerWindow = nsPIDOMWindowOuter::From(aWindow);
>+}
I'm not quite happy with this setup, but don't have better suggestion now.
I think I'd be happier if we had some kind of union { frameLoader, pendingOpenerWindow };

You must traverse/unlink mOpenerWindow in nsGenericHTMLFrameElement.
>+nsXULElement::PresetOpenerWindow(mozIDOMWindowProxy* aWindow, ErrorResult& aRv)
>+{
>+    nsXULSlots* slots = static_cast<nsXULSlots*>(Slots());
>+
>+    // Cannot SetOpenerWindow when a frame loader is present
>+    if (NS_WARN_IF(slots->mFrameLoader)) {
You may crash here.
>@@ -612,16 +613,17 @@ protected:
>     {
>     public:
>         nsXULSlots();
>         virtual ~nsXULSlots();
> 
>         void Traverse(nsCycleCollectionTraversalCallback &cb);
> 
>         RefPtr<nsFrameLoader> mFrameLoader;
>+        nsCOMPtr<mozIDOMWindowProxy> mOpener;
Also here it would be quite nice to be able to use some kind of union. It would be easier to understand, IMO, and would use less memory.


I understand maybe 1/3 of this patch, so this will need couple of iterations.
Attachment #8798992 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #19)
> If you name the method DocGroup() and not GetDocGroup(), you're hinting to
> the caller the method never returns null.
> So, you shouldn't need to have this if-check here.
> But, looks like DocGroup() can actually return null, at least in case of
> data documents (document.implementation.createDocument("", ""))
> So, rename the method to GetDocGroup() (or make it somehow infallible)

Changed.

> I don't understand this method. Couldn't you just merge this to
> SetOpenerWindow?

I didn't do this originally in order to prevent semantic changes, but as it is only exposed to chrome code it should be fine.

> >+nsGlobalChromeWindow::SetInitialOpenerWindow(mozIDOMWindowProxy* aOpenerWindow)
> >+{
> >+  MOZ_ASSERT(!mInitialOpenerWindow);
> >+  mInitialOpenerWindow = aOpenerWindow;
> >+  return NS_OK;
> >+}
> >+
> >+nsresult
> >+nsGlobalChromeWindow::TakeInitialOpenerWindow(mozIDOMWindowProxy** aOpenerWindow)
> >+{
> >+  // Intentionally forget our own member
> >+  mInitialOpenerWindow.forget(aOpenerWindow);
> >+  return NS_OK;
> >+}
> Why do we need this initial opener stuff?

In order to get the properties for DocGroups which we want, we need to make sure that they are never changed. We connect the DocGroup to a document when the Document is added to the window, which is basically when it is created. When creating a window through binding a xul:browser to the document, these both happen synchronously. We need to set the opener on the window after the window is created, but before the document is. These APIs, along with the PresetOpenerWindow APIs, exist to tell the xul:browser what its opener should be, such that it can set it in nsXULElement::LoadSrc after the window is created but before the document.

In this specific case, InitialOpenerWIndow is used for new windows, where we currently synchronously create the entire <tabbrowser> which creates the xul:browser and binds it. We set the initial opener on the chrome window which holds the tabbrowser such that we can have the information early enough.

> I'm not quite happy with this setup, but don't have better suggestion now.
> I think I'd be happier if we had some kind of union { frameLoader,
> pendingOpenerWindow };
> 
> You must traverse/unlink mOpenerWindow in nsGenericHTMLFrameElement.

> Also here it would be quite nice to be able to use some kind of union. It
> would be easier to understand, IMO, and would use less memory.

I agree that a union will be nicer. I havent changed that in the current patch because it will be a bit of work. I'll get it done before we land this patch, but I'm hoping right now that my explanations help you review the rest of the patch.

> I understand maybe 1/3 of this patch, so this will need couple of iterations.

Fair.
Attachment #8798992 - Attachment is obsolete: true
Comment on attachment 8798991 [details] [diff] [review]
Part 1: Add the DocGroup and TabGroup Objects

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

Do you have a purpose in mind for mWindows?

::: dom/base/DocGroup.cpp
@@ +36,5 @@
> +
> +DocGroup::DocGroup(TabGroup* aTabGroup, const nsACString& aKey)
> +  : mKey(aKey), mTabGroup(aTabGroup)
> +{
> +  // This method does not add itself to mTabGroup->mDocGroups as the caller does it for us

End with a period.

::: dom/base/DocGroup.h
@@ +27,5 @@
> +// a "unit of similar-origin related browsing contexts"
> +//
> +// A TabGroup is a set of browsing contexts which are all "related". Within a
> +// TabGroup, browsing contexts are broken into "similar-origin" DocGroups. In
> +// more detail, a DocGroup is actually a collection of inner windows, and a

This comment needs to be updated to reflect the fact that a DocGroup contains documents.

@@ +81,5 @@
> +private:
> +  class HashEntry : public nsCStringHashKey
> +  {
> +  public:
> +    // NOTE: Weak reference. The DocGroup destructor removes itself from itw

itw -> its

@@ +106,5 @@
> +  already_AddRefed<DocGroup>
> +  GetDocGroup(const nsACString& aKey);
> +
> +  already_AddRefed<DocGroup>
> +  JoinDocGroup(const nsACString& aKey, nsIDocument* aDocument);

Please comment what JoinDocGroup does. Since you're also using the word Join for something else, it might be good to rename this. Perhaps AddDocument (since you use Remove for the opposite operation on DocGroup)?
Attachment #8798991 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #22)
> Comment on attachment 8798991 [details] [diff] [review]
> Part 1: Add the DocGroup and TabGroup Objects
> 
> Review of attachment 8798991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you have a purpose in mind for mWindows?

Yes. I hope to use it both in my 32-bit OOM system to determine whether or not there are related windows which can prevent a window from performing a process switch, and I was thinking it would be useful for telemetry purposes.

> Please comment what JoinDocGroup does. Since you're also using the word Join
> for something else, it might be good to rename this. Perhaps AddDocument
> (since you use Remove for the opposite operation on DocGroup)?

That sounds like a better name. I also felt weird about using Join() twice.
Comment on attachment 8799895 [details] [diff] [review]
Part 2: Connect the DocGroup and TabGroup objects to nsGlobalWindow and nsDocument, ensuring that Opener is set early enough that it is correct

+            // We don't want to do anything if our browser is already remote,
+            // however, if we have an opener set we want to make sure that
+            // the browser is bounced in order to set it!
+            if (aOpener && aShouldBeRemote) {
+              throw new Exception("Cannot set an opener on a browser which should be remote!");
+            }
             let isRemote = aBrowser.getAttribute("remote") == "true";
-            if (isRemote == aShouldBeRemote)
+            if (isRemote == aShouldBeRemote) {
+              if (aOpener && aBrowser.contentWindow.opener != aOpener) {
+                throw new Exception("What?");
+              }
So this is a bit hard to read. Could you just have one 'if' which validates the arguments and aBrowser state, and throws if there is something unexpected.
In particular if (isRemote == aShouldBeRemote) { hints that the 'if' after that might apply to case when aShouldBeRemote is true, but apparently it doesn't, since there is also another
check at the beginning of the method. 


>+            // Set the opener window on the browser, such that when the frame
>+            // loader is created the opener is set correctly.
>+            aBrowser.presetOpenerWindow(aOpener ? aOpener : null);
Just pass aOpener?

+  if (window) {
+    // We should already have the principal, and now that we have been added to a
+    // document, we should be able to join a docGroup!
+    nsAutoCString docGroupKey;
+    mozilla::dom::DocGroup::GetKey(NodePrincipal(), docGroupKey);
+    if (mDocGroup) {
+      MOZ_RELEASE_ASSERT(mDocGroup->MatchesKey(docGroupKey));
+    } else {
+      mozilla::dom::TabGroup* tabgroup = nsGlobalWindow::Cast(window)->TabGroup();
+      mDocGroup = tabgroup->JoinDocGroup(docGroupKey, this);
+    }
+  }
I think it would be nicer if all the documents actually were in doc group, not only non-data-documents.
So, join to a group when scope object is set?



>+  // Check that the content visible opener matches!
"content visible". content is so overloaded word, so perhaps use something else.
"js visible" ?

>+nsGlobalWindow::GetSanitizedOpener(nsPIDOMWindowOuter* aOpener, bool aCallerIsChrome)
I'm having trouble to understand from the name of the method how/why aCallerIsChrome affects to the behavior
of the method.
Would it make sense to do the chrome check in nsGlobalWindow::GetOpenerWindowOuter(), and then just remove aCallerIsChrome argument.

>   nsWeakPtr mFullscreenPresShell;
>+  nsCOMPtr<mozIDOMWindowProxy> mInitialOpenerWindow;
You should traverse/unlink mInitialOpenerWindow


>@@ -149,16 +151,25 @@ nsGenericHTMLFrameElement::EnsureFrameLoader()
>   }
> 
>   // Strangely enough, this method doesn't actually ensure that the
>   // frameloader exists.  It's more of a best-effort kind of thing.
>   mFrameLoader = nsFrameLoader::Create(this, mNetworkCreated);
>   if (mIsPrerendered) {
>     mFrameLoader->SetIsPrerendered();
>   }
>+  if (mOpenerWindow) {
>+    nsCOMPtr<nsIDocShell> docShell;
>+    mFrameLoader->GetDocShell(getter_AddRefs(docShell));
>+    if (docShell) {
>+      nsCOMPtr<nsPIDOMWindowOuter> outerWindow = docShell->GetWindow();
>+      outerWindow->SetOpenerWindow(nsPIDOMWindowOuter::From(mOpenerWindow), true);
>+      mOpenerWindow = nullptr;
>+    }
>+  }
This feels a bit risky.  It forces docshell creation when we're possibly still inside script blocker.
Could you rather just pass the opener to nsFrameLoader and it would use it when docshell is created.

>+        nsCOMPtr<nsPIDOMWindowOuter> opener;
>+        if (slots->mOpener) {
>+            opener = nsPIDOMWindowOuter::From(slots->mOpener);
>+            slots->mOpener = nullptr;
>+        } else {
>+            // If we are a content-primary xul-browser, we want to take the opener property!
>+            nsCOMPtr<nsIDOMChromeWindow> chromeWindow = do_QueryInterface(OwnerDoc()->GetWindow());
>+            if (AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
>+                            NS_LITERAL_STRING("content-primary"), eIgnoreCase) &&
>+                chromeWindow) {
>+                nsCOMPtr<mozIDOMWindowProxy> wp;
>+                chromeWindow->TakeInitialOpenerWindow(getter_AddRefs(wp));
>+                opener = nsPIDOMWindowOuter::From(wp);
>+            }
>+        }
>+
>+        if (opener) {
>+            nsCOMPtr<nsIDocShell> docShell;
>+            nsresult rv = slots->mFrameLoader->GetDocShell(getter_AddRefs(docShell));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+            nsCOMPtr<nsPIDOMWindowOuter> outerWindow = docShell->GetWindow();
>+            outerWindow->SetOpenerWindow(opener, true);
Same applies here.

>@@ -612,16 +613,17 @@ protected:
>     {
>     public:
>         nsXULSlots();
>         virtual ~nsXULSlots();
> 
>         void Traverse(nsCycleCollectionTraversalCallback &cb);
> 
>         RefPtr<nsFrameLoader> mFrameLoader;
>+        nsCOMPtr<mozIDOMWindowProxy> mOpener;
Could you prepare perhaps a separate patch/bug to make this union. We don't want to increase memory use of most of the xul elements just because of this, IMO

>+            if (parentWindow) {
>+              nsCOMPtr<nsIDocShellTreeItem> nsdsti=
>+                do_GetInterface(newChrome);
>+              nsCOMPtr<nsIWebNavigation> webNav =
>+                do_GetInterface(nsdsti);
>+              webNav->Stop(nsIWebNavigation::STOP_NETWORK);
I don't understand this.
Attachment #8799895 - Flags: review?(bugs) → review-
Comment on attachment 8798996 [details] [diff] [review]
Part 3: Update the named window resolution logic to be scoped to TabGroup instead of being process-global

Sorry for the lag here; this didn't feel like a review I wanted to rush through.

>+++ b/docshell/base/nsDocShell.cpp
>@@ -3675,57 +3675,34 @@ nsDocShell::FindItemWithName(const char16_t* aName,
>+    if (name.LowerCaseEqualsLiteral("_self") ||
>+        name.LowerCaseEqualsLiteral("_content") ||
>+        name.EqualsLiteral("_main")) {

No, this isn't right.  In content docshells (i.e. on the web) we currently have "_main" and "_content" behave like "_top", effectively.  In chrome code we have them target the currently selected tab in non-e10s and they're broken in e10s (always give a new window, afaict).  In either case they do not act like "_self".

The spec doesn't have "_main" and "_content" at all, and looks like at least Chrome doesn't support them (so they always give a new , so maybe we can just drop them altogether?  That would be safer for possible chrome/extension consumers too: instead of navigating themselves they'd just get a new window.

I did just check our codebase, and the only place where these appear other than the code you're changing seems to be the contentAreaClick function in browser.js.  It's possible that that code can get simplified a bit as a followup.  Maybe.  I say that because I do see lots of hits at https://dxr.mozilla.org/addons/search?q=%27%22_main%22%27&redirect=false and some at https://dxr.mozilla.org/addons/search?q=%27%22_content%22%27 that seem relevant.

It may be worth double-checking with a Firefox peer what the situation is with this thing and whether we have any test for what these extensions are doing with these targets... especially because we _haven't_ yet rolled e10s out to users with extensions.

>@@ -3789,33 +3766,37 @@ nsDocShell::DoFindItemWithName(const char16_t* aName,
>+    if (!GetIsMozBrowserOrApp() && parentAsTreeItem->ItemType() == mItemType) {

Hmm....

I was going to suggest you just use GetSameTypeParent here, but that doesn't quite work because:

1)  This would break the aRequestor check, since nsDocShell::FindChildWithName can in fact cross type boundaries if aSameType is false.

2)  This would also break the aRequestor check because nsDocShell::FindChildWithName doesn't actually stop at mozbrowser/app boundaries.

In both cases you would end up calling to the docgroup instead of returning null as now...

I think #2 is just buggy.  I strongly suspect the existence of the aSameType thing is also buggy and we should always behave as if it were true.  Please file a followup bug on all that and add a comment in FindChildWithName referencing that bug and pointing out that if this is all fixed then this code can just get simplified to use GetSameTypeParent.

>+  // If we have a null parent or the parent is of the same type, we need to

"is not of the same type".

>+  // give up on finding it in our tree, and start looking in our TabGroup.
>+  nsCOMPtr<nsPIDOMWindowOuter> window = GetWindow();
>+  if (window) {
>+    RefPtr<mozilla::dom::TabGroup> tabGroup =
>+      nsGlobalWindow::Cast(window)->TabGroup();

This is a little silly, because the mScriptGlobal we store is already nsGlobalWindow, so we're casting it to nsPIDOMWindowOuter* and then back.

It's OK for now, I guess, but I would almost rather we had GetTabGroup() separately on inner/outer windows (with the former delegating to the latter) so we could just get one off a nsPIDOMWindowOuter/nsPIDOMWindowInner).  The setup these patches are adding is sort of a step backwards on finishing the inner/outer separation...

Please file a followup bug on the way TabGroup() is exposed on windows to move it to the nsPI* classes.

>+++ b/dom/base/DocGroup.cpp
>+TabGroup::FindItemWithName(const char16_t* aName,
>+  // The names "_blank", "_content", and "_main" mean that we should simply open
>+  // in a new window, so return nothing.

Well.. .that's not what "_content" and "_main" mean in your docshell changes above!  Please have these be consistent.

I think the right thing is to not special-case these two names at all in this code.

Bonus points for a followup to change the name from char16_t to a const nsAString& here and elsewhere.

>+  if (name.LowerCaseEqualsLiteral("_blank") ||

Can this code actually be reached with the name "_blank"?  Seems wrong/buggy to me for that to happen...

Likewise, it should never be reached with "_top" or "_parent" or "_self".

Or are your worried about calls from the windowwatcher?  Seems to me like we should handle those special cases in the windowwatcher, then.

>+++ b/dom/base/DocGroup.h
>+  nsresult
>+  FindItemWithName(const char16_t* aName,
>+                   nsIDocShellTreeItem* aRequestor,
>+                   nsIDocShellTreeItem* aOriginalRequestor,
>+                   nsIDocShellTreeItem** aFoundItem);

Needs documentation.

>+++ b/xpfe/appshell/nsChromeTreeOwner.cpp
>-      mXULWindow->GetPrimaryContentShell(aFoundItem);

We may be able to nix the concept of primary content shells and GetPrimaryContentShell.  Followup bug, please.

>+++ b/xpfe/appshell/nsContentTreeOwner.cpp
>-           do_QueryReferent(mXULWindow->mTargetableShells[i]);

I believe after this mTargetableShells is write-only, so we can nix it and all its management.  We should also be able to nix GetTargetableShellCount(), the aTargetable argument to ContentShellAdded, the special-casing of "content-targetable" in the callers of ContentShellAdded, and the use of "content-targetable" in our chrome.  Followup bug, please.

r=me with the above issues addressed, though I would like to see the updated patch.
Attachment #8798996 - Flags: review?(bzbarsky) → review+
Attached patch Part 1: Add the DocGroup and TabGroup Objects (obsolete) (deleted) — — Splinter Review 
MozReview-Commit-ID: ILXWUlPPeHM
Attachment #8798991 - Attachment is obsolete: true
Attachment #8798996 - Attachment is obsolete: true
Attachment #8799895 - Attachment is obsolete: true
MozReview-Commit-ID: 46GtT8uYTtP
Attachment #8801369 - Flags: review?(bugs)
MozReview-Commit-ID: AKse7r2JKm3
Attachment #8801370 - Flags: review?(ehsan)
Can't r? you so a ni? will have to do :D
Flags: needinfo?(bzbarsky)
Comment on attachment 8801367 [details] [diff] [review]
Part 2: Connect the DocGroup and TabGroup objects to nsGlobalWindow and nsDocument, ensuring that Opener is set early enough that it is correct

>@@ -4680,16 +4715,30 @@ nsDocument::SetScriptHandlingObject(nsIScriptGlobalObject* aScriptObject)
> {
>   NS_ASSERTION(!mScriptGlobalObject ||
>                mScriptGlobalObject == aScriptObject,
>                "Wrong script object!");
>   if (aScriptObject) {
>     mScopeObject = do_GetWeakReference(aScriptObject);
>     mHasHadScriptHandlingObject = true;
>     mHasHadDefaultView = false;
>+
>+    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aScriptObject);
>+    if (window) {
>+      // We should already have the principal, and now that we have been added to a
>+      // window, we should be able to join a DocGroup!
>+      nsAutoCString docGroupKey;
>+      mozilla::dom::DocGroup::GetKey(NodePrincipal(), docGroupKey);
>+      if (mDocGroup) {
>+        MOZ_RELEASE_ASSERT(mDocGroup->MatchesKey(docGroupKey));
>+      } else {
>+        mozilla::dom::TabGroup* tabgroup = nsGlobalWindow::Cast(window)->TabGroup();
>+        mDocGroup = tabgroup->AddDocument(docGroupKey, this);
You may crash here. Unlike its name hints, TabGroup() called on inner window may return null.


It is a bit unclear to me why you do this here and not in SetScopeObject (and update this code to call SetScopeObject).
But in practice this should be fine too.

>+nsGlobalWindow::TabGroup()
> {
>-  FORWARD_TO_INNER_VOID(GetConstellation, (aConstellation));
>+  FORWARD_TO_OUTER(TabGroup, (), nullptr);
So, the method may return null. Call it GetTabGroup() and make sure all the callers null check the return value.

>+    // NOTE: It is not possible to have a scriptableparentornull which is
>+    // `this`, but it is possible to have opener be `this`, so we need to check
>+    // for that and ignore that case.
Check what and where? Is this some leftover code or is there something missing?
Or maybe I just misunderstand the comment

>+    } else {
>+      // Sanity check that our tabgroup matches our opener or parent.
>+      RefPtr<nsPIDOMWindowOuter> parent = GetScriptableParentOrNull();
>+      MOZ_ASSERT_IF(parent, Cast(parent)->TabGroup() == mTabGroup);
>+      nsCOMPtr<nsPIDOMWindowOuter> piOpener = do_QueryReferent(mOpener);
>+      nsGlobalWindow* opener = Cast(GetSanitizedOpener(piOpener));
>+      MOZ_ASSERT_IF(opener && opener != this, opener->TabGroup() == mTabGroup);
>+    }
Is the comment above about these assertions? If so, better to move it here


>+    mHasValidatedTabGroup = false;
I don't understand the name of this variable. Why you set it false after validation?
Should it be called 'mIsValidatingTabGroup' or what?

>   mFrameLoader = nsFrameLoader::Create(this, mNetworkCreated);
>   if (mIsPrerendered) {
>     mFrameLoader->SetIsPrerendered();
>   }
>+  if (mOpenerWindow) {
>+    nsCOMPtr<nsIDocShell> docShell;
>+    mFrameLoader->GetDocShell(getter_AddRefs(docShell));
>+    if (docShell) {
>+      nsCOMPtr<nsPIDOMWindowOuter> outerWindow = docShell->GetWindow();
>+      outerWindow->SetOpenerWindow(nsPIDOMWindowOuter::From(mOpenerWindow), true);
>+      mOpenerWindow = nullptr;
>+    }
>+  }
I think it would have been clearer to pass mOpenerWindow to ::Create and then let frameloader to
use that when it creates docshell. But I guess this works too.
>@@ -1577,16 +1580,40 @@ nsXULElement::LoadSrc()
>     if (!slots->mFrameLoader) {
>         // false as the last parameter so that xul:iframe/browser/editor
>         // session history handling works like dynamic html:iframes.
>         // Usually xul elements are used in chrome, which doesn't have
>         // session history at all.
>         slots->mFrameLoader = nsFrameLoader::Create(this, false);
>         NS_ENSURE_TRUE(slots->mFrameLoader, NS_OK);
> 
>+        nsCOMPtr<nsPIDOMWindowOuter> opener;
>+        if (slots->mOpener) {
>+            opener = nsPIDOMWindowOuter::From(slots->mOpener);
>+            slots->mOpener = nullptr;
>+        } else {
>+            // If we are a content-primary xul-browser, we want to take the opener property!
>+            nsCOMPtr<nsIDOMChromeWindow> chromeWindow = do_QueryInterface(OwnerDoc()->GetWindow());
>+            if (AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
>+                            NS_LITERAL_STRING("content-primary"), eIgnoreCase) &&
Hmm, so this is for the case when a new window is opened and it has just one tab, right?
Feels rather hackish, but I know we do something similar with e10s new window opening when passing right TabParent or so.

>+                nsCOMPtr<mozIDOMWindowProxy> wp;
>+                chromeWindow->TakeInitialOpenerWindow(getter_AddRefs(wp));
>+                opener = nsPIDOMWindowOuter::From(wp);
>+            }
>+        }
>+
>+        if (opener) {
>+            nsCOMPtr<nsIDocShell> docShell;
>+            nsresult rv = slots->mFrameLoader->GetDocShell(getter_AddRefs(docShell));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+            nsCOMPtr<nsPIDOMWindowOuter> outerWindow = docShell->GetWindow();
>+            outerWindow->SetOpenerWindow(opener, true);
>+        }
What guarantees the content-primary window gets a non-chrome window as opener?


>@@ -612,16 +613,17 @@ protected:
>     {
>     public:
>         nsXULSlots();
>         virtual ~nsXULSlots();
> 
>         void Traverse(nsCycleCollectionTraversalCallback &cb);
> 
>         RefPtr<nsFrameLoader> mFrameLoader;
>+        nsCOMPtr<mozIDOMWindowProxy> mOpener;
Still this? Why? I thought we agreed that this so rarely used property shouldn't be added to already heavy slots.


Some clarifications needed.
Attachment #8801367 - Flags: review?(bugs) → review-
ah, there is a separate patch for slots
Comment on attachment 8801369 [details] [diff] [review]
Part 4: Merge the mFrameLoader and mOpener into mFrameLoaderOrOpener

># HG changeset patch
># User Michael Layzell <michael@thelayzells.com>
>
>Bug 1303196 - Part 4: Merge the mFrameLoader and mOpener into mFrameLoaderOrOpener, r=smaug
>
>MozReview-Commit-ID: 46GtT8uYTtP
>
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>index ea544b3..4f904b9 100644
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -3675,21 +3675,17 @@ nsDocShell::FindItemWithName(const char16_t* aName,
>     return DoFindItemWithName(aName, aRequestor, aOriginalRequestor, aResult);
>   } else {
>     // This is the entry point into the target-finding algorithm.  Check
>     // for special names.  This should only be done once, hence the check
>     // for a null aRequestor.
> 
>     nsCOMPtr<nsIDocShellTreeItem> foundItem;
>     nsDependentString name(aName);
>-    // _main is an IE target which should be case-insensitive but isn't
>-    // see bug 217886 for details
>-    if (name.LowerCaseEqualsLiteral("_self") ||
>-        name.LowerCaseEqualsLiteral("_content") ||
>-        name.EqualsLiteral("_main")) {
>+    if (name.LowerCaseEqualsLiteral("_self")) {
How is this change related to this patch?

> TabGroup::FindItemWithName(const char16_t* aName,
>                            nsIDocShellTreeItem* aRequestor,
>                            nsIDocShellTreeItem* aOriginalRequestor,
>                            nsIDocShellTreeItem** aFoundItem)
> {
>   NS_ENSURE_ARG_POINTER(aFoundItem);
>   *aFoundItem = nullptr;
> 
>-
>-  // The names "_blank", "_content", and "_main" mean that we should simply open
>-  // in a new window, so return nothing.
>-  nsDependentString name(aName);
>-  if (name.LowerCaseEqualsLiteral("_blank") ||
>-      name.LowerCaseEqualsLiteral("_content") ||
>-      name.EqualsLiteral("_main")) {
>-    return NS_OK;
>-  }
>-
ditto




>+  // An opener window which should be used when the docshell is created.
>+  nsCOMPtr<nsPIDOMWindowOuter> mOpener;
>+
This must be cycle collected 

>     nsXULSlots* slots = static_cast<nsXULSlots*>(GetExistingDOMSlots());
>     if (slots) {
>         NS_IF_RELEASE(slots->mControllers);
>-        if (slots->mFrameLoader) {
>-            // This element is being taken out of the document, destroy the
>-            // possible frame loader.
>-            // XXXbz we really want to only partially destroy the frame
>-            // loader... we don't want to tear down the docshell.  Food for
>-            // later bug.
>-            slots->mFrameLoader->Destroy();
>-            slots->mFrameLoader = nullptr;
>+        RefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+        if (frameLoader) {
>+            frameLoader->Destroy();
>+            slots->mFrameLoaderOrOpener = nullptr;
You should always set the variable to null, not only when it is a frameloader

> nsXULElement::DestroyContent()
> {
>     nsXULSlots* slots = static_cast<nsXULSlots*>(GetExistingDOMSlots());
>     if (slots) {
>         NS_IF_RELEASE(slots->mControllers);
>-        if (slots->mFrameLoader) {
>-            slots->mFrameLoader->Destroy();
>-            slots->mFrameLoader = nullptr;
>+        RefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+        if (frameLoader) {
>+            frameLoader->Destroy();
>+            slots->mFrameLoaderOrOpener = nullptr;
ditto. Other we might get wrong opener if someone decides to reuse the element later
>@@ -2061,16 +2061,21 @@ nsWindowWatcher::FindItemWithName(const char16_t* aName,
>                                   nsIDocShellTreeItem* aOriginalRequestor,
>                                   nsIDocShellTreeItem** aFoundItem)
> {
>   *aFoundItem = nullptr;
>   if (!aName || !*aName) {
>     return NS_OK;
>   }
> 
>+  nsDependentString name(aName);
>+  if (name.LowerCaseEqualsLiteral("_blank")) {
>+    return NS_OK;
>+  }
What has this to do with this patch?



r- because the patch seems to have seemingly unrelated changes.
Attachment #8801369 - Flags: review?(bugs) → review-
It seems like I really screwed up my rebases at 7pm on friday, so I'm clearing review flags until I get some fixed up patches up.
Flags: needinfo?(bzbarsky)
(In reply to Olli Pettay [:smaug] from comment #32)
> You may crash here. Unlike its name hints, TabGroup() called on inner window
> may return null.

It can only fail if you don't have an outer window. I've changed the code to add a MOZ_ASSERT with an explanation.

> It is a bit unclear to me why you do this here and not in SetScopeObject
> (and update this code to call SetScopeObject).
> But in practice this should be fine too.

Changed. I agree that that makes more sense.

> Check what and where? Is this some leftover code or is there something
> missing?
> Or maybe I just misunderstand the comment

I forgot to remove the comment when I removed the code. Sorry about that.

> I don't understand the name of this variable. Why you set it false after
> validation?
> Should it be called 'mIsValidatingTabGroup' or what?

Yes. It should.

> I think it would have been clearer to pass mOpenerWindow to ::Create and
> then let frameloader to
> use that when it creates docshell. But I guess this works too.

In the follow up merge mFrameLoader and mOpener into mFrameLoaderOrOpener patch that happens. Unfortunately I made the changes at the same time, and didn't want to pull them apart, but at the same time I didn't want to add the complexity of that patch to this patch, so this is done in two phases.

> >@@ -1577,16 +1580,40 @@ nsXULElement::LoadSrc()
> >     if (!slots->mFrameLoader) {
> >         // false as the last parameter so that xul:iframe/browser/editor
> >         // session history handling works like dynamic html:iframes.
> >         // Usually xul elements are used in chrome, which doesn't have
> >         // session history at all.
> >         slots->mFrameLoader = nsFrameLoader::Create(this, false);
> >         NS_ENSURE_TRUE(slots->mFrameLoader, NS_OK);
> > 
> >+        nsCOMPtr<nsPIDOMWindowOuter> opener;
> >+        if (slots->mOpener) {
> >+            opener = nsPIDOMWindowOuter::From(slots->mOpener);
> >+            slots->mOpener = nullptr;
> >+        } else {
> >+            // If we are a content-primary xul-browser, we want to take the opener property!
> >+            nsCOMPtr<nsIDOMChromeWindow> chromeWindow = do_QueryInterface(OwnerDoc()->GetWindow());
> >+            if (AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
> >+                            NS_LITERAL_STRING("content-primary"), eIgnoreCase) &&
> Hmm, so this is for the case when a new window is opened and it has just one
> tab, right?
> Feels rather hackish, but I know we do something similar with e10s new
> window opening when passing right TabParent or so.

e10s new window opening is more sketchy. There is a RAII guard which sets a static which is used to discover which TabGroup to use. I was originally considering doing a similar thing, but this is much less likely to horribly break in the future I would think.

> >+                nsCOMPtr<mozIDOMWindowProxy> wp;
> >+                chromeWindow->TakeInitialOpenerWindow(getter_AddRefs(wp));
> >+                opener = nsPIDOMWindowOuter::From(wp);
> >+            }
> >+        }
> >+
> >+        if (opener) {
> >+            nsCOMPtr<nsIDocShell> docShell;
> >+            nsresult rv = slots->mFrameLoader->GetDocShell(getter_AddRefs(docShell));
> >+            NS_ENSURE_SUCCESS(rv, rv);
> >+            nsCOMPtr<nsPIDOMWindowOuter> outerWindow = docShell->GetWindow();
> >+            outerWindow->SetOpenerWindow(opener, true);
> >+        }
> What guarantees the content-primary window gets a non-chrome window as
> opener?

If a chrome window is the opener of a content window, that fact is hidden from content in nsGlobalWindow::GetSanitizedOpener. I _think_ I only set the opener right now in cases where it would have been set anyways.

> Still this? Why? I thought we agreed that this so rarely used property
> shouldn't be added to already heavy slots.

See the other patch.
Comment on attachment 8801370 [details] [diff] [review]
Part 5: Disable a window namespacing test due to window namespacing behavior changes

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

Why disable when you can remove the test?  :-)
Attachment #8801370 - Flags: review?(ehsan) → review-
MozReview-Commit-ID: ILXWUlPPeHM
Attachment #8801366 - Attachment is obsolete: true
Attachment #8801367 - Attachment is obsolete: true
Attachment #8801368 - Attachment is obsolete: true
Attachment #8801369 - Attachment is obsolete: true
Attachment #8801370 - Attachment is obsolete: true
Updated based on feedback. I _think_ I rebased it correctly this time, so you shouldn't see a ton of unrelated changes anymore.
Attachment #8801792 - Flags: review?(bugs)
MozReview-Commit-ID: DuJRM5KsGhb
Attachment #8801794 - Flags: review?(bugs)
Attachment #8801792 - Attachment is obsolete: true
Attachment #8801792 - Flags: review?(bugs)
MozReview-Commit-ID: AKse7r2JKm3
Attachment #8801808 - Flags: review?(ehsan)
Flags: needinfo?(bzbarsky)
Comment on attachment 8801793 [details] [diff] [review]
Part 3: Update the named window resolution logic to be scoped to TabGroup instead of being process-global

>+TabGroup::FindItemWithName(const char16_t* aName,

It might be good to assert that the incoming name is not _blank/_top/_parent/_self, right?  Note that those are supposed to be case-insensitive compares.

And update the docs accordingly.

And I guess update the windowwatcher caller too, to do the early return on _self/_top/_parent, not just _blank.  People should really not be asking it for those anyway.

r=me with that done and those followup bugs I asked for in my previous review all filed.
Flags: needinfo?(bzbarsky)
Attachment #8801793 - Flags: review+
Attachment #8801793 - Attachment is obsolete: true
Comment on attachment 8801808 [details] [diff] [review]
Part 5: Remove a window namespacing test due to window namespacing behavior changes

ehsan r+-ed in person
Attachment #8801808 - Flags: review?(ehsan) → review+
Attachment #8801794 - Flags: review?(bugs) → review+
Comment on attachment 8801807 [details] [diff] [review]
Part 2: Connect the DocGroup and TabGroup objects to nsGlobalWindow and nsDocument, ensuring that Opener is set early enough that it is correct

>+nsresult
>+nsGlobalChromeWindow::SetInitialOpenerWindow(mozIDOMWindowProxy* aOpenerWindow)
>+{
>+  MOZ_ASSERT(!mInitialOpenerWindow);
>+  mInitialOpenerWindow = aOpenerWindow;
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsGlobalChromeWindow::TakeInitialOpenerWindow(mozIDOMWindowProxy** aOpenerWindow)
>+{
>+  // Intentionally forget our own member
>+  mInitialOpenerWindow.forget(aOpenerWindow);
>+  return NS_OK;
>+}
These need assertions telling whether these are supposed to be called on inner or outer (outer).

>+nsGlobalWindow::TabGroupInner()
>+{
Please document why this method is infallible,  what guaranteess this is always called before
outer window goes away.

>   // Specify that we want the window to remain locked until the chrome has loaded.
>   nsXULWindow *xulWin = static_cast<nsXULWindow*>
>                                    (static_cast<nsIXULWindow*>
>                                                (newWindow));
> 
>+  if (aOpener) {
>+    nsCOMPtr<nsIDocShell> docShell;
>+    xulWin->GetDocShell(getter_AddRefs(docShell));
>+    MOZ_ASSERT(docShell);
>+    nsCOMPtr<nsIDOMChromeWindow> chromeWindow =
>+      do_QueryInterface(docShell->GetWindow());
>+    MOZ_ASSERT(chromeWindow);
>+
>+    chromeWindow->SetInitialOpenerWindow(aOpener);
Now that I think of. I think the method name isn't clear enough, since the opener is really only about
Nothing in SetInitialOpenerWindow hints that it is for the case when a new window with a content docshell is about to be loaded.
So, SetOpenerForInitialContentBrowser() perhaps?
Attachment #8801807 - Flags: review?(bugs) → review+
I was getting some android test failures because they handle opening windows differently. I believe that this patch will fix them.
Attachment #8803039 - Flags: review?(bugs)
Attachment #8803039 - Flags: review?(bugs) → review+
After the noopener changes I had to make some changes as opener is now set differently. This patch makes those changes.
Attachment #8804358 - Flags: review?(bugs)
This patch is going to break addon compat a bit. The following are the big changes:

* The function nsIBrowserDOMWindow::openURI has been removed, and replaced with nsIBrowserDOMWIndow::openURIWithOpener (I changed the name to make it easier for addons to detect which version was avaliable). Addons mostly seem to override this method in order to try to control our tab opening behavior, such as where we open a new tab. The new function takes one additional argument on top of the old version.

* The special named window names "_main" and "_content" have been removed, as they are not part of the spec, and other browsers such as chrome don't support them. Browsers which use those targets will now create a new window instead of their previous behaviors. These window targets were already broken in e10s (as in, in e10s they open new windows).

:jorge, are these breakages OK?
Flags: needinfo?(jorge)
(In reply to Michael Layzell [:mystor] from comment #50) 
> * The special named window names "_main" and "_content" have been removed,
> as they are not part of the spec, and other browsers such as chrome don't
> support them. Browsers which use those targets will now create a new window
> instead of their previous behaviors. These window targets were already
> broken in e10s (as in, in e10s they open new windows).

That change should be done in a different bug, right?
(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #51)
> (In reply to Michael Layzell [:mystor] from comment #50) 
> > * The special named window names "_main" and "_content" have been removed,
> > as they are not part of the spec, and other browsers such as chrome don't
> > support them. Browsers which use those targets will now create a new window
> > instead of their previous behaviors. These window targets were already
> > broken in e10s (as in, in e10s they open new windows).
> 
> That change should be done in a different bug, right?

I was talking with bz on IRC earlier today and we agreed that it _should_ have been done in a different bug, but that there is little point in seperating it out now (It is a prerequisite for part 3 of this patch). http://logs.glob.uno/?c=content#c403360
It looks from the patch that aOpener is an optional argument. Is there a reason it can't be added as the last argument and the function name maintained so that compatibility isn't broken?
Flags: needinfo?(jorge)
Keywords: addon-compat
Comment on attachment 8804358 [details] [diff] [review]
Part 7: Updates to apply correctly after noopener changes

>-  openURI: function (aURI, aOpener, aWhere, aContext) {
>+  openURIWithOpener: function (aURI, aParent, aOpener, aWhere, aContext) {
I don't quite like the new naming here. aParent really is the opener, and aOpener is like some flag whether opener should be set on the newly opened window.
aOpener is either null or aParent.
I think if a const was defined in nsIBrowserDOMWindow and passed around telling how to use aOpener, this would become easier to understand.
And fewer changes to browser.js
Attachment #8804358 - Flags: review?(bugs) → review-
(In reply to Jorge Villalobos [:jorgev] from comment #53)
> It looks from the patch that aOpener is an optional argument. Is there a
> reason it can't be added as the last argument and the function name
> maintained so that compatibility isn't broken?

Unfortunately, most consumers of this API don't actually just call the function, but rather replace it with another function which wraps it and then proxies to our implementation. If they did that and we had an optional last argument instead, it would silently succeed and cause opener to not be set on new windows as they are created, as the addon would fail to pass one of the arguments through. I explicitly broke the API because I was worried about that breaking web compat.
Flags: needinfo?(jorge)
Attachment #8801807 - Attachment is obsolete: true
(In reply to Michael Layzell [:mystor] from comment #55)
> I explicitly broke the API because I was worried
> about that breaking web compat.

Okay, that makes sense. I think it's okay then.
Flags: needinfo?(jorge)
MozReview-Commit-ID: LGHTayzJDOG
Attachment #8804358 - Attachment is obsolete: true
This new version no longer should break addons! I have repurposed the aContext argument as a bitflags argument which allows me to pass in the required information without requiring additional arguments.
Flags: needinfo?(jorge)
r? smaug
Flags: needinfo?(bugs)
Comment on attachment 8804903 [details] [diff] [review]
Part 7: Updates to apply correctly after noopener changes

>     if (aOpener && isExternal) {
>-      Cu.reportError("nsBrowserAccess.openURI did not expect an opener to be " +
>+      Cu.reportError("nsBrowserAccess.openURI did not expect an parent to be " +
s/parent/opener/

>+++ b/browser/base/content/test/general/browser_bug537474.js
>@@ -1,8 +1,8 @@
> add_task(function *() {
>   let browserLoadedPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
>   browserDOMWindow.openURI(makeURI("about:"), null,
>-                           Ci.nsIBrowserDOMWindow.OPEN_CURRENTWINDOW, null)
>+                           Ci.nsIBrowserDOMWindow.OPEN_CURRENTWINDOW, null);
unrelated change.

>+++ b/devtools/client/responsive.html/browser/tunnel.js
>@@ -223,18 +223,18 @@ function tunnelToInnerBrowser(outer, inner) {
>       let { detail } = event;
>       event.preventDefault();
>       let uri = Services.io.newURI(detail.url, null, null);
>       // This API is used mainly because it's near the path used for <a target/> with
>       // regular browser tabs (which calls `openURIInFrame`).  The more elaborate APIs
>       // that support openers, window features, etc. didn't seem callable from JS and / or
>       // this event doesn't give enough info to use them.
>       browserWindow.browserDOMWindow
>-                   .openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB,
>-                            Ci.nsIBrowserDOMWindow.OPEN_NEWTAB);
>+        .openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB,
>+                 Ci.nsIBrowserDOMWindow.OPEN_NEW);
unrelated change

>   bool isTargetTopLevelDocShell = false;
>   nsCOMPtr<nsIDocShell> targetDocShell;
>   if (!aWindowTarget.IsEmpty()) {
>     // Locate the target DocShell.
>     nsCOMPtr<nsIDocShellTreeItem> targetItem;
>-    nsDependentString name(aWindowTarget);
>     // Only _self, _parent, and _top are supported in noopener case.
>     if (!(aFlags & INTERNAL_LOAD_FLAGS_NO_OPENER) ||
>-        name.LowerCaseEqualsLiteral("_self") ||
>-        name.LowerCaseEqualsLiteral("_parent") ||
>-        name.LowerCaseEqualsLiteral("_top")) {
>+        aWindowTarget.LowerCaseEqualsLiteral("_self") ||
>+        aWindowTarget.LowerCaseEqualsLiteral("_parent") ||
>+        aWindowTarget.LowerCaseEqualsLiteral("_top")) {
unrelated change? Though, perhaps some other patch here touches aWindowTarget, in which case this is fine


>         rv = win->Open(NS_ConvertUTF8toUTF16(spec),
>-                       name, // window name
>+                       aWindowTarget, // window name
ditto

>+++ b/dom/base/nsHostObjectURI.cpp
>@@ -3,16 +3,17 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsHostObjectURI.h"
> 
> #include "nsIObjectInputStream.h"
> #include "nsIObjectOutputStream.h"
>+#include "nsHostObjectProtocolHandler.h"
this is to fix some compilation issue

>-   * Open in the "current window".  If aOpener is provided, this should be the
>-   * top window in aOpener's window hierarchy, but exact behavior is
>-   * application-dependent.  If aOpener is not provided, it's up to the
>+   * Open in the "current window".  If aParent is provided, this should be the
>+   * top window in aParent's window hierarchy, but exact behavior is
>+   * application-dependent.  If aParent is not provided, it's up to the
>    * application to decide what constitutes a "current window".
Leftover comment change from the previous patch? It shouldn't start talking about parent.

>   /**
>-   * Values for openURI's aContext parameter.  These affect the behavior of
>-   * OPEN_DEFAULTWINDOW.
>+   * Values for openURI's aFlags parameter. This is a bitflags field.
>+   *
>+   * The 0x1 bit decides the behavior of OPEN_DEFAULTWINDOW, and the 0x4 bit
>+   * controls whether or not to set the window.opener property on the newly
>+   * opened window.
>+   *
>+   * NOTE: The 0x2 bit is ignored for backwards compatibility with addons, as
>+   * OPEN_NEW used to have the value 2.
Could you still add a comment that 2 and 0 are treated the same way internally


>         if ( bwin ) {
>           nsCOMPtr<nsIURI> uri;
>           NS_NewURI( getter_AddRefs( uri ), NS_LITERAL_CSTRING("about:blank"), 0, 0 );
>           if ( uri ) {
>             nsCOMPtr<mozIDOMWindowProxy> container;
>-            rv = bwin->OpenURI( uri, 0,
>-                                nsIBrowserDOMWindow::OPEN_DEFAULTWINDOW,
>-                                nsIBrowserDOMWindow::OPEN_EXTERNAL,
>-                                getter_AddRefs( container ) );
>+            rv = bwin->OpenURIWithOpener( uri, nullptr,
>+                                          nsIBrowserDOMWindow::OPEN_DEFAULTWINDOW,
>+                                          nsIBrowserDOMWindow::OPEN_EXTERNAL,
>+                                          getter_AddRefs( container ) );
back to the original name
Flags: needinfo?(bugs)
Attachment #8804903 - Flags: review+
(In reply to Michael Layzell [:mystor] from comment #59)
> This new version no longer should break addons! I have repurposed the
> aContext argument as a bitflags argument which allows me to pass in the
> required information without requiring additional arguments.

Awesome, thanks!
Flags: needinfo?(jorge)
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe339dd59a01
Part 1: Add the DocGroup and TabGroup Objects, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d65b1e1a76
Part 2: Connect the DocGroup and TabGroup objects to nsGlobalWindow and nsDocument, ensuring that Opener is set early enough that it is correct, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee25aab52f5
Part 3: Update the named window resolution logic to be scoped to TabGroup instead of being process-global, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3515eb43f05a
Part 4: Merge mFrameLoader and mOpener into mFrameLoaderOrOpener, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f778f3ea30
Part 5: Remove a window namespacing test due to window namespacing behavior changes, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc29cabb300b
Part 6: Correctly set the opener when creating new tabs on android, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ef5b08ae25
Part 7: Updates to apply correctly after noopener changes, r=smaug
Blocks: 1316389
Blocks: 1316561
Depends on: 1334086
Depends on: 1334047
Depends on: 1362661
Depends on: 1402253
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: