1277028 - [e10s] HTTP redirect to external protocol doesn't work in e10s

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Direct loads of something like <iframe src="mailto:"> work, but <iframe src="http://whatever"> that does a 3xx redirect to an external protocol just shows an error page.  This works correctly without e10s.

May be related to bug 590682.

Comment 1

[Jim Mathies [:jimm]]

Hanza, curious if you think bug 590682 will fix this?

Also, would you consider this an e10s blocker?

Comment 2

[Honza Bambas (:mayhemer)]

Definitely fixes that.  W/o the patch: Corrupted Content Error, w/ the patch: redirect works (mailto: pops up with the expected dialog)

Comment 3

[Honza Bambas (:mayhemer)]

I'm afraid duplication was too premature.  The patch in bug 590682 had an issue, that when fixed, reverts the redirect to problem described in this bug.

Hence, I'm reopening it.

We can first go with a workaround - instead of CORRUPTED_CONTENT error (and its error page) we can leave the redirect source page which will probably contain the target link that users may click manually.  How complicated this workaround would be implement I cannot say at the moment.

Comment 4

[Jim Mathies [:jimm]]

Per the necko backlog flag here, we're not going to block e10s on this. Please renom if you disagree.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1 (use the universal proxy for redirected-to channel when a channel doesn't impl nsIChildChannel)

- this is using the code from our famous bug 590682
- when the channel created with request to redirect on the child process doesn't support nsiChildChannel, use the proxy instead
- this forwards most external protocols

Disadvantage: breaks redirects to web registered protocols.  Hence f? only.

Comment 7

[Honza Bambas (:mayhemer)]

With patch from bug 590682 (v1.3.1) and this patch applied redirect to mailto: links still doesn't work.  I manage to open the choose dialog, but pressing the [Open Link] button leads to a following JS exception:

JavaScript error: .../components/nsWebHandlerApp.js, line 112: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIURILoader.openURI]


The dialog._windowCtxt is not what's expected.  Expected is docshell, but here it's HttpChannelParentListener.  GI to nsIURIContentListener fails when called from nsDocumentOpenInfo::Prepare().

The choose dialog is initialized somewhere from here after the redirect is confirmed (happens on the parent process):

>	xul.dll!nsURILoader::OpenURI(0x137b6604, 0, 0x13ab1418) Line 797	C++
 	xul.dll!nsDocShell::DoChannelLoad(0x137b6604, 0x147addc0, false) Line 11234	C++
 	xul.dll!nsDocShell::DoURILoad(0x12672200, 0x00000000, true, 0, 0x0de754c0, 0x00000000, {...}, 0x00000000, 0x00000000, true, 0x00000000, 0x006fc8fc, true, false, false, {...}, 0x00000000, 6) Line 11048	C++
 	xul.dll!nsDocShell::InternalLoad(0x12672200, 0x00000000, 0x00000000, 0, 0x0de754c0, 8, 0x118a1210, 0x00000000, {...}, 0x00000000, 0x00000000, 1, 0x00000000, true, {...}, 0x00000000, 0x00000000, 0x00000000, 0x00000000) Line 10522	C++
 	xul.dll!nsDocShell::LoadURI(0x12672200, 0x1243c760, 16384, true) Line 1542	C++
 	xul.dll!nsWindowWatcher::OpenWindowInternal(0x00000000, 0x1439c7f0, 0x00000000, 0x1439c820, false, true, true, 0x00000000, 0x135193a0, 0x00000000, 0x006fd408) Line 1066	C++
 	xul.dll!nsWindowWatcher::OpenWindow(0x00000000, 0x1439c7f0, 0x00000000, 0x1439c820, 0x135193a0, 0x006fd408) Line 369	C++
0 ask(aHandler = [xpconnect wrapped nsIHandlerInfo @ 0x1432c6d0 (native @ 0x11daa4c0)], aWindowContext = [xpconnect wrapped nsIInterfaceRequestor @ 0x1432c700 (native @ 0x12989120)], aURI = [xpconnect wrapped nsIURI @ 0x1432c7c0 (native @ 0x12351830)], aReason = 0) ["file:///c:/Mozilla/src/mozilla-central/_obj-browser-debug/dist/bin/components/nsContentDispatchChooser.js":72]
    this = [object Object]
 	xul.dll!nsExternalHelperAppService::LoadURI(0x11deff10, 0x12989120) Line 1074	C++
 	xul.dll!nsExtProtocolChannel::OpenURL() Line 158	C++
 	xul.dll!nsExtProtocolChannel::AsyncOpen(0x131a2a6c, 0x00000000) Line 200	C++
 	xul.dll!mozilla::net::nsHttpChannel::ContinueProcessRedirection(NS_OK) Line 5125	C++
 	xul.dll!mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(NS_OK) Line 7165	C++
 	xul.dll!mozilla::net::nsAsyncVerifyRedirectCallbackEvent::Run() Line 41	C++
 	xul.dll!nsThread::ProcessNextEvent(false, 0x006ff565) Line 1073	C++


In the direct load case is tho initialized from elsewhere (also on parent):

>	xul.dll!nsURILoader::OpenURI(0x124ed6d4, 0, 0x1233a018) Line 797	C++
 	xul.dll!nsDocShell::DoChannelLoad(0x124ed6d4, 0x147addc0, false) Line 11234	C++
 	xul.dll!nsDocShell::DoURILoad(0x11afdfc0, 0x00000000, true, 0, 0x0de754c0, 0x00000000, {...}, 0x00000000, 0x00000000, true, 0x00000000, 0x006faa24, true, false, false, {...}, 0x00000000, 6) Line 11048	C++
 	xul.dll!nsDocShell::InternalLoad(0x11afdfc0, 0x00000000, 0x00000000, 0, 0x0de754c0, 8, 0x0def97e8, 0x00000000, {...}, 0x00000000, 0x00000000, 1, 0x00000000, true, {...}, 0x00000000, 0x00000000, 0x00000000, 0x00000000) Line 10522	C++
 	xul.dll!nsDocShell::LoadURI(0x11afdfc0, 0x11daa4c0, 16384, true) Line 1542	C++
 	xul.dll!nsWindowWatcher::OpenWindowInternal(0x00000000, 0x133b9700, 0x00000000, 0x133b97c0, false, true, true, 0x00000000, 0x13098d00, 0x00000000, 0x006fb530) Line 1066	C++
 	xul.dll!nsWindowWatcher::OpenWindow(0x00000000, 0x133b9700, 0x00000000, 0x133b97c0, 0x13098d00, 0x006fb530) Line 369	C++
 	xul.dll!_NS_InvokeByIndex() Line 57	Unknown
 	xul.dll!CallMethodHelper::Invoke() Line 2075	C++
 	xul.dll!CallMethodHelper::Call() Line 1394	C++
 	xul.dll!XPCWrappedNative::CallMethod(, ) Line 1361	C++
 	xul.dll!XPC_WN_CallMethod(0x0deee000, 5, 0x11ac20a0) Line 1128	C++
 	xul.dll!js::CallJSNative(0x0deee000, 0x02bcf58e, {...}) Line 227	C++
 	xul.dll!js::InternalCallOrConstruct(0x0deee000, {...}, NO_CONSTRUCT) Line 452	C++
 	xul.dll!InternalCall(0x0deee000, {...}) Line 497	C++
 	xul.dll!js::CallFromStack(0x0deee000, {...}) Line 503	C++
 	xul.dll!Interpret(, ) Line 2872	C++
 	xul.dll!js::RunScript(0x0deee000, {...}) Line 398	C++
 	xul.dll!js::InternalCallOrConstruct(0x0deee000, {...}, NO_CONSTRUCT) Line 470	C++
 	xul.dll!InternalCall(0x0deee000, {...}) Line 497	C++
 	xul.dll!js::Call(0x0deee000, {...}, {...}, {...}, {...}) Line 516	C++
 	xul.dll!JS_CallFunctionValue(0x0deee000, {...}, {...}, {...}, {...}) Line 2799	C++
 	xul.dll!nsXPCWrappedJSClass::CallMethod(0x12324080, 3, 0x0de77000, 0x006fd1b8) Line 1211	C++
 	xul.dll!nsXPCWrappedJS::CallMethod(3, 0x0de77000, 0x006fd1b8) Line 615	C++
 	xul.dll!PrepareAndDispatch(0x13098cd0, 3, 0x006fd264, 0x006fd254) Line 85	C++
 	xul.dll!SharedStub() Line 113	C++
 	xul.dll!nsExternalHelperAppService::LoadURI(0x11d48c40, 0x11d43624) Line 1074	C++
 	xul.dll!mozilla::dom::ContentParent::RecvLoadURIExternal({...}, 0x2146a400) Line 4424	C++

Which is invoked by the ext protocol channel's call on nsExternalHelperAppService::LoadURI that redirects to parent.




I'll try to go a different way here - make the ext proto channel IPC aware (impl nsIChildChannel and nsIParentChannel) and when redirected to, proxy to child (or something like that) to do the actual ext uri load from the child side.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v2 (teach extprotocol channel to be redirected to in e10s)

Turns out to be pretty easy.  I just make nsExtProtocolChannel implement both nsIParentChannel and nsIChildChannel, which makes it be put in the redirect registrar and lets the wheels spin as expected.  There is only one added async IPC message in PContent.

Ext protocol handling works well when invoked plainly from child process because there is a branch that forwards to the parent process in nsExternalHelperAppService::LoadURI.  Why then not to go from parent directly?  Because we then don't have the right docshell!  When we make the (a) child channel be called AsyncOpen on it, we get the right result and the select dialog works as expected, no NO_INTERFACE from nsURILoader::OpenURI -> nsDocumentOpenInfo::Prepare.

Comment 10

[Jason Duell]

Comment on attachment 8765978 [details] [diff] [review]
v2 (teach extprotocol channel to be redirected to in e10s)

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

Simpler than I expected, though still slightly funky.

Do we have any automated tests for this (or a way to write them)?  I understand if it's hard to do with external apps.  I assume you've at least test it by hand :)

::: uriloader/exthandler/nsExternalProtocolHandler.cpp
@@ +63,5 @@
>      nsLoadFlags mLoadFlags;
>      bool mWasOpened;
> +    // Set true when this channel is a parent process redirect target
> +    // channel connected as itself.  It then prevents AsyncOpen from
> +    // doing its job, since we do it on the child process.

// Set true when this channel is on the parent process and it being used as 
// a redirect target channel.  It turns AsyncOpen into a no-op since we do it on the child.

@@ +372,5 @@
> +
> +NS_IMETHODIMP nsExtProtocolChannel::CompleteRedirectSetup(nsIStreamListener *listener,
> +                                                          nsISupports *context)
> +{
> +  return AsyncOpen(listener, context);

add comment above AsyncOpen call:

  // For redirects to external protocols we AsyncOpen on the child (not the parent)
  // because it has the right docshell (which is needed for the select dialog)

@@ +406,5 @@
> +NS_IMETHODIMP nsExtProtocolChannel::OnStartRequest(nsIRequest *aRequest,
> +                                                   nsISupports *aContext)
> +{
> +  // no data is expected
> +  return NS_ERROR_UNEXPECTED;

I think we should use MOZ_RELEASE_ASSERT(), for all these cases since we really don't ever expect them to be called, right?  It would be a major code error for these to be called IIUC.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #10)
> Comment on attachment 8765978 [details] [diff] [review]
> v2 (teach extprotocol channel to be redirected to in e10s)
> 
> Review of attachment 8765978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Simpler than I expected, though still slightly funky.

Out e10s redirects are funky, that's true.

> 
> Do we have any automated tests for this (or a way to write them)?  I
> understand if it's hard to do with external apps.  I assume you've at least
> test it by hand :)

I've spent a lot of time checking this manually.  Maybe there could be a test made, we have some mailto tests I think that I few times broke, but honestly, I don't want to waste in this case time with writing a test.  This should better be given to QA for manual testing, which will be better in this case.

> I think we should use MOZ_RELEASE_ASSERT(), for all these cases since we
> really don't ever expect them to be called, right?  It would be a major code
> error for these to be called IIUC.

I added MOZ_CRASH'es, yes.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v2.1 (teach extprotocol channel to be redirected to in e10s)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca8b9d8abd1
Make external protocol handlers work with e10s redirects, r=jduell

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ca8b9d8abd1

Comment 15

[Dietrich Ayala (:dietrich)]

Verified fixed on Mac. Vox -> Soundcloud in browser -> Vox roundtrip works.