ContentParent::RecvCreateWindowInDifferentProcess doesn't propagate OriginAttributes or window.name

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bobowen, Assigned: Nika)

Tracking

(Blocks: 1 bug)

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: sbwc3)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year 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.
(Assignee)

Comment 1

a year 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.
Assignee: nobody → michael
Blocks: 1365032
Summary: ContentParent::RecvCreateWindowInDifferentProcess doesn't propagate userContextId or private browsing → ContentParent::RecvCreateWindowInDifferentProcess doesn't propagate OriginAttributes or window.name
(Assignee)

Comment 2

a year ago
We should also make sure that sandboxing flags are propagated as well.
(Assignee)

Comment 3

a year ago
Created attachment 8874986 [details] [diff] [review]
Part 1: Propagate window.name across processes for RecvCreateWindowInDifferentProcess

MozReview-Commit-ID: 6xmLN9pbCKd
Attachment #8874986 - Flags: review?(bugs)
(Assignee)

Comment 4

a year ago
Created attachment 8874987 [details] [diff] [review]
Part 2: Propagate OriginAttributes across processes for RecvCreateWindowInDifferentProcess

MozReview-Commit-ID: 8ok4DI9zgfR
Attachment #8874987 - Flags: review?(bugs)
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.
Attachment #8874986 - Flags: review?(bugs) → review+
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.
Attachment #8874987 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

a year ago
Created attachment 8875320 [details] [diff] [review]
Part 1: Propagate window.name across processes for RecvCreateWindowInDifferentProcess

MozReview-Commit-ID: 6xmLN9pbCKd
(Assignee)

Updated

a year ago
Attachment #8874986 - Attachment is obsolete: true
Attachment #8874987 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Created attachment 8875321 [details] [diff] [review]
Part 2: Propagate OriginAttributes across processes for RecvCreateWindowInDifferentProcess

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
Attachment #8875321 - Flags: review?(bugs)
Comment on attachment 8875321 [details] [diff] [review]
Part 2: Propagate OriginAttributes across processes for RecvCreateWindowInDifferentProcess

yeah, file a followup for the frontend code.
Attachment #8875321 - Flags: review?(bugs) → review+

Comment 10

a year 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
(Assignee)

Updated

a year ago
Blocks: 1370969

Updated

a year ago
Blocks: 1371100
https://hg.mozilla.org/mozilla-central/rev/548be4ca230b
https://hg.mozilla.org/mozilla-central/rev/8dd6234e2a18
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.