Closed Bug 1338375 Opened 3 years ago Closed 3 years ago

Opening in a new http(s) window broken from file URL pages

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- disabled
firefox54 --- verified
firefox55 --- verified

People

(Reporter: billm, Assigned: bobowen)

References

Details

(Whiteboard: sbwc2, sbmc2, sblc3)

Attachments

(2 files, 3 obsolete files)

STR:
1. Start a fresh profile.
2. In preferences, uncheck "Open new windows in a new tab instead" (in General).
3. Open a file: URL that contains a target="_blank" link.
4. Click on the link.

ER: The link opens in a new window.
AR: A blank window opens with no browser chrome.
(In reply to Bill McCloskey (:billm) from comment #0)
> STR:
> 1. Start a fresh profile.
> 2. In preferences, uncheck "Open new windows in a new tab instead" (in
> General).
> 3. Open a file: URL that contains a target="_blank" link.
> 4. Click on the link.
> 
> ER: The link opens in a new window.
> AR: A blank window opens with no browser chrome.

I imagine that this is related to https://bugzilla.mozilla.org/show_bug.cgi?id=1338241 - I might end up fixing it in my patch for that bug tomorrow.
See Also: → 1338241
Sorry, I forgot to mention that I bisected. This was regressed by bug 1147911. So I don't think bug 1338241.
Looks like I never tested this particular case.

If the URL is a file:// URL it works, if it is an http(s):// URL and we're going through the new RecvCreateWindowInDifferentProcess function, looks like we don't attempt to navigate the new window, doh!
Assignee: nobody → bobowencode
Blocks: 1321430
Status: NEW → ASSIGNED
Summary: Opening in a new window broken for file URLs → Opening in a new http(s) window broken from file URLs pages
Whiteboard: sbwc2, sbmc2, sblc3
Summary: Opening in a new http(s) window broken from file URLs pages → Opening in a new http(s) window broken from file URL pages
Comment on attachment 8836740 [details] [diff] [review]
Part 1: Fix window settings and navigation when creating a new window in a new process

So you remove the only caller of OpenWindowWithoutParent. Please remove that.

Why OpenURI takes null as opener?
Attachment #8836740 - Flags: review?(bugs) → review-
Comment on attachment 8836741 [details] [diff] [review]
Part 2: Check that new http browser window opened from file URI page works

please test also <a target=_blank>
Attachment #8836741 - Flags: review?(bugs) → review-
Thanks for the reviews.

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8836740 [details] [diff] [review]
> Part 1: Fix window settings and navigation when creating a new window in a
> new process
>
> So you remove the only caller of OpenWindowWithoutParent. Please remove that.

Removed.

> Why OpenURI takes null as opener?

Originally, I'd put this in the else when aSetOpener was false anyway.
At the moment aURIToLoad is only set when aSetOpener is false, but I suppose it doesn't hurt to allow for potential future uses.
Attachment #8837122 - Flags: review?(bugs)
Attachment #8836740 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8836741 [details] [diff] [review]
> Part 2: Check that new http browser window opened from file URI page works
> 
> please test also <a target=_blank>

Test with link added.
Attachment #8837123 - Flags: review?(bugs)
Attachment #8836741 - Attachment is obsolete: true
Comment on attachment 8837122 [details] [diff] [review]
Part 1: Fix window settings and navigation when creating a new window in a new process

>+  if (aURIToLoad) {
>+    nsCOMPtr<mozIDOMWindowProxy> openerWindow;
>+    if (aSetOpener && thisTabParent) {
>+      openerWindow = thisTabParent->GetParentWindowOuter();
>+    }
>+    nsCOMPtr<nsIBrowserDOMWindow> browserDOMWin =
>+      TabParent::GetFrom(aNewTabParent)->GetBrowserDOMWindow();
I would null check GetBrowserDOMWindow
Attachment #8837122 - Flags: review?(bugs) → review+
Attachment #8837123 - Flags: review?(bugs) → review+
Attachment #8837122 - Attachment is obsolete: true
Comment on attachment 8841510 [details] [diff] [review]
Part 1: Fix window settings and navigation when creating a new window in a new process

Thanks smaug.

Carrying r+ from comment 11.

I've pushed to try again as it's been a while:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cac71c70aaea9fc6f2bab2f3ec4f8eaebbb5e7bd

(I've fixed that trailing space issue highlighted in the Lint build locally.)

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8837122 [details] [diff] [review]

> >+    nsCOMPtr<nsIBrowserDOMWindow> browserDOMWin =
> >+      TabParent::GetFrom(aNewTabParent)->GetBrowserDOMWindow();
> I would null check GetBrowserDOMWindow

Added null check similar to the one above, also noticed that I was shadowing browserDOMWindow, so I changed that name.
Attachment #8841510 - Flags: review+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a009a402bd99
Part 1: Fix window settings and navigation when creating a new window in a new process.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08710fe142c8
Part 2: Check that new http browser window opened from file URI page works.
https://hg.mozilla.org/mozilla-central/rev/a009a402bd99
https://hg.mozilla.org/mozilla-central/rev/08710fe142c8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reproduced the initial issue on Nightly 54.0a1 (Build ID: 20170209030214) on Windows 10 x64.

Verified as fixed using the latest Nightly 55.0a1 - Build ID: 20170323030203 and Aurora 54.0a2 - Build ID: 20170323004002 on Windows 10 x64, Ubuntu 16.04 x64 and Mac Os X 10.12
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.