Bob Owen (:bobowen) Reporter
	Description • 7 years ago

+++ This bug was initially created as a clone of Bug #1364879 +++

Filing this as a separate bug, because bug 1364879 was fixed when we flipped the pref to allow related HTTP pages to run in the file content process for bug 1351358.

I still think we should fix this, but it's less urgent as this code is not currently used with the default configuration.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 1 • 7 years ago

I want to use RecvCreateWindowInDifferentProcess to move noopener-ed URLs into a separate process, and so I'm going to need to fix these things.

I also need to fix propagating window.name to the newly opened window.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 2 • 7 years ago

We should also make sure that sandboxing flags are propagated as well.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 3 • 7 years ago

MozReview-Commit-ID: 6xmLN9pbCKd

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 4 • 7 years ago

MozReview-Commit-ID: 8ok4DI9zgfR

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 7 years ago

Comment on attachment 8874986 [details] [diff] [review]
Part 1: Propagate window.name across processes for RecvCreateWindowInDifferentProcess

>@@ -2053,16 +2056,24 @@
>               // Gecko is going to read this attribute and use it.
>               b.setAttribute("nextTabParentId", aParams.nextTabParentId.toString());
>             }
> 
>             if (aParams.sameProcessAsFrameLoader) {
>               b.sameProcessAsFrameLoader = aParams.sameProcessAsFrameLoader;
>             }
> 
>+            // This will be used by gecko to control the name of the opened
>+            // window.
>+            if (aParams.name) {
>+              // XXX: The `name` property is special in HTML and XUL. Should
>+              // we use a different attribute name for this?
>+              b.setAttribute("name", aParams.name);
well, <html:iframe name=foo> passes foo as the name of the window inside the iframe. So 'name' looks reasonable.

>+  /**
>+   * Checks if the passed-in name is one of the legal values for window.name.
>+   * Illegal values include: "", "_blank", "_top", "_parent" and "_self".
>+   */
>+  static bool IsLegalWindowName(const nsAString& aName);
I don't understand this. "" is perfectly legal name.
Should this method be called something like
IsOverridingWindowName()



>+  MOZ_ASSERT(aNewTabParent);
>+  // If we were passed a valid string as the name for the window, we should send
>+  // it back up.
>+  if (!aName.IsEmpty() &&
>+      !aName.LowerCaseEqualsLiteral("_blank") &&
>+      !aName.LowerCaseEqualsLiteral("_top") &&
>+      !aName.LowerCaseEqualsLiteral("_parent") &&
>+      !aName.LowerCaseEqualsLiteral("_self")) {
Hmm, this kind of check again. Please deduplicate
Also, this part could use a comment why we want to explicitly pass name.
Something about OpenWindowWithTabParent being tricky to modify for that.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 7 years ago

Comment on attachment 8874987 [details] [diff] [review]
Part 2: Propagate OriginAttributes across processes for RecvCreateWindowInDifferentProcess

I think I'd prefer propagating whole OA, that should be hopefully less error prone in case someone adds new attributes.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 7 • 7 years ago

MozReview-Commit-ID: 6xmLN9pbCKd

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 8 • 7 years ago

I changed the message on TabParent to send the full OriginAttributes down. I wasn't sure what I should do to the frontend code which is invoked with the OriginAttributes in the new tab case. Currently the only property of the OAs which is accessed in the new tab case is the userContextId, and the rest of the OA is discarded. I could try to change that to also propagate other properties to new tabs, but I feel like perhaps that should be done in a follow-up?

Not sure, would appreciate your opinions.

MozReview-Commit-ID: 8ok4DI9zgfR

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 7 years ago

Comment on attachment 8875321 [details] [diff] [review]
Part 2: Propagate OriginAttributes across processes for RecvCreateWindowInDifferentProcess

yeah, file a followup for the frontend code.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/548be4ca230b
Part 1: Propagate window.name across processes for RecvCreateWindowInDifferentProcess, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd6234e2a18
Part 2: Propagate OriginAttributes across processes for RecvCreateWindowInDifferentProcess, r=smaug

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/548be4ca230b
https://hg.mozilla.org/mozilla-central/rev/8dd6234e2a18