1253307 - [e10s] Opening in a new tab/window a link handled by a web protocol handler shows the wrong URL

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Florian Quèze [:florian]]

This is very similar to bug 1116478.

Steps to reproduce:
1. Load https://people.mozilla.org/~fqueze2/tmp/registerProtocolhandler.html
2. Click on the "click here to register a test protocol handler" line.
3. Click the "Add Application" button in the notification bar.
4. Go to about:preferences#applications
5. Find the "webcal" type in the list, and set the default application to "Test"
6. On the page loaded at step #1, open the "test webcal link" in a new tab (either using Cmd+click, or the "Open Link in New Tab" context menu item).

Expected result:
A new tab with this URL:
https://people.mozilla.org/~fqueze2/tmp/registerProtocolhandler.html?s=webcal%3Ahttp%3A%2F%2Fexample.org%2F
The favicon should be the favicon of https://people.mozilla.org/ (ie. a Firefox logo).

Actual result with e10s enabled (in a non-e10s window, the bug can't be reproduced):
A new tab with "webcal:http://example.org/" in the URL bar and no favicon.


Note: if opening the link in a new window instead of in a new tab, the URL bar shows "about:blank".

I think the reason why this wasn't noticed while fixing bug 1116478 is that the STRs there involve using Gmail, which immediately redirects; hiding the problem.

Comment 1

[Florian Quèze [:florian]]

I wrote a test for this in bug 1254118 (contains several |todo()| and |if (browser.isRemote)| that should be removed once this bug is fixed).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I don't know why I invented OpenURI instead of using LoadURL in the first place. I think I was worried about not passing the |flags| argument properly through but testing shows that it isn't important (and code inspection also shows that it isn't needed in this case). Much easier.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Test fix

Florian's test hasn't landed yet but these are the changes to it for when it does.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f700add7f88

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8729710 [details] [diff] [review]
Patch v1

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

Generally this seems like a good thing. I had forgotten about LoadURL. I have two questions though.

First, why does this fix the problem?

Second, I'm concerned about the initialization-time stuff that RecvLoadURL does. Normally I think we only call this function when the frame loader is created. The process name code does some debug stuff that seems wrong at this time, and I don't know what the consequences of re-running that service worker registration stuff are. Maybe not a big deal, but it might be nice to have some static vars to make sure this stuff happens only once.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bill McCloskey (:billm) from comment #5)
> First, why does this fix the problem?

When I wrote the patch in bug 1116478, I missed that I had to set LOAD_DOCUMENT_URI on the channel's loadFlags before calling into nsURILoader. In fact, if I add that, this bug is also fixed. Would you prefer that patch or the patch here with statics for the initialization stuff?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

The other major difference between the two approaches is that I can't find where we call content security checks for the nsURILoader version. I think I prefer adding statics to protect against re-initialization and keeping this patch's general direction.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #7)
> The other major difference between the two approaches is that I can't find
> where we call content security checks for the nsURILoader version. I think I
> prefer adding statics to protect against re-initialization and keeping this
> patch's general direction.

Yeah, sounds good to me.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.5

I surrounded all of the init code in RecvLoadURL with a static protector.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4ec703a35cf

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8729710 [details] [diff] [review]
Patch v1

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

This seems fine. I haven't done the deep-dive that you have regarding not passing the flags, but I trust you.

My only complaint is that we're keeping the aFlags argument to the nsIRemoteWindowContext OpenURI call. Should probably just wipe that out if we're ignoring it.

::: dom/ipc/ContentParent.cpp
@@ +1598,5 @@
>    return QueryInterface(aIID, aSink);
>  }
>  
>  NS_IMETHODIMP
>  RemoteWindowContext::OpenURI(nsIURI* aURI, uint32_t aFlags)

If we're just ignoring this flags argument, we should probably just remove it from the method signature.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> If we're just ignoring this flags argument, we should probably just remove
> it from the method signature.

This is implementing an IDL interface that's implemented elsewhere (and doesn't ignore the flags argument) so I can't just remove it, sadly.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.6

I was confused, here's a patch with mconley's suggestion.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8730475 [details] [diff] [review]
Patch v1.6

LGTM, thanks!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/70b8ca40c091

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70b8ca40c091

Comment 17

[Chris Peterson [:cpeterson]]

Blake, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8730475 [details] [diff] [review]
Patch v1.6

Approval Request Comment
[Feature/regressing bug #]: bug 1116478
[User impact if declined]: web protocol handlers opened in new tabs will have uninitialized UI elements.
[Describe test coverage new/current, TreeHerder]: There are tests on Treeherder (thanks Florian!).
[Risks and why]: Low risk.
[String/UUID change made/needed]: n/a

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Blake, could this impact non-e10s behavior? I just want to make sure even though we will uplift this to 47 for e10s experiments, it should not pose a risk to the non-e10s behavior (default in 47).

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Ritu Kothari (:ritu) from comment #19)
> Hi Blake, could this impact non-e10s behavior? I just want to make sure even
> though we will uplift this to 47 for e10s experiments, it should not pose a
> risk to the non-e10s behavior (default in 47).

No, this is change to code that only runs in e10s builds.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #20)
> (In reply to Ritu Kothari (:ritu) from comment #19)
> > Hi Blake, could this impact non-e10s behavior? I just want to make sure even
> > though we will uplift this to 47 for e10s experiments, it should not pose a
> > risk to the non-e10s behavior (default in 47).
> 
> No, this is change to code that only runs in e10s builds.

Got it. Thanks for the confirmation. Will approve the uplift.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730475 [details] [diff] [review]
Patch v1.6

e10s related, Aurora47+

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f7a5afcc407