1380186 - SimpleChannel needs Parent/Child IPC

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Shane Caraveo (:mixedpuppy)]

To properly support http redirects, we need SimpleChannel (and thus moz-ext protocol) to implement nsIChildChannel.

Comment 1

[Shane Caraveo (:mixedpuppy)]

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1380186 implement SimpleChannel Parent/Child IPC

Review commit: https://reviewboard.mozilla.org/r/156684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156684/

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885904 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156684/diff/1-2/

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1380186 test http redirects to moz-ext protocol,

Review commit: https://reviewboard.mozilla.org/r/156688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156688/

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review commit: https://reviewboard.mozilla.org/r/156690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156690/

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

The redirect tests in the other patch pass with this, but I'm not 100% sure about things.  try is running.  would like to get an initial round of feedback.

Comment 9

[Shane Caraveo (:mixedpuppy)]

maybe tests will run this time.

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

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Well, that try is looking pretty good, so I think I'll just move this to a review request.

Kris r? for webext stuff
Honza r? for http channel redirect stuff

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

https://reviewboard.mozilla.org/r/156690/#review162038

lgtm, with few formatting nits fixed (probably only an f? thing)

::: netwerk/base/SimpleChannel.cpp:15
(Diff revision 1)
> +#include "nsIChannel.h"
> +#include "nsISupportsImpl.h"
>  #include "nsBaseChannel.h"
>  #include "nsIInputStream.h"
>  #include "nsIRequest.h"
> -#include "SimpleChannel.h"
> +#include "nsIChildChannel.h"

added twice, please use alphabetical order (except the main SimpleChannel.h file).

::: netwerk/base/SimpleChannel.cpp:95
(Diff revision 1)
>    return NS_OK;
>  }
>  
>  #undef TRY_VAR
>  
> +class SimpleChannelChild : public SimpleChannel

final?

::: netwerk/base/SimpleChannel.cpp:100
(Diff revision 1)
> +class SimpleChannelChild : public SimpleChannel
> +                         , public nsIChildChannel
> +                         , public PSimpleChannelChild
> +{
> +public:
> +    explicit SimpleChannelChild(UniquePtr<SimpleChannelCallbacks>&& aCallbacks);

please use two spaces indent.

::: netwerk/base/SimpleChannel.cpp:113
(Diff revision 1)
> +private:
> +    ~SimpleChannelChild();
> +
> +    void AddIPDLReference();
> +
> +    bool mIPCOpen;

please assert that manipulations with this flag are all happening on the main thread only

::: netwerk/base/SimpleChannelParent.h:18
(Diff revision 1)
> +#include "mozilla/net/PSimpleChannelParent.h"
> +
> +namespace mozilla {
> +namespace net {
> +
> +// In order to support HTTP redirects to data:, we need to implement the HTTP

update the comment

::: netwerk/base/SimpleChannelParent.h:22
(Diff revision 1)
> +
> +// In order to support HTTP redirects to data:, we need to implement the HTTP
> +// redirection API, which requires a class that implements nsIParentChannel
> +// and which calls NS_LinkRedirectChannels.
> +class SimpleChannelParent : public nsIParentChannel
> +                        , public PSimpleChannelParent

indent, not only here
use two spaces

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/1-2/

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/1-2/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

https://reviewboard.mozilla.org/r/156690/#review163724

r=me with issues addressed.

But, at this point, we should probably also make nsDataChannel just use SimpleChannel, and get rid of all of its duplicated parent/child gunk. Can you do that in a follow-up?

::: netwerk/base/SimpleChannel.cpp:106
(Diff revision 2)
> +
> +protected:
> +  virtual void ActorDestroy(ActorDestroyReason why) override;
> +
> +private:
> +  ~SimpleChannelChild();

Nit: `virtual ~SimpleChannelChild() = default;`

::: netwerk/base/SimpleChannel.cpp:145
(Diff revision 2)
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +SimpleChannelChild::CompleteRedirectSetup(nsIStreamListener *aListener,
> +                                        nsISupports *aContext)

Nit: Indentation. And `*` gets attached to the type, not the parameter name.

::: netwerk/base/SimpleChannel.cpp:151
(Diff revision 2)
> +  }
> +  else {

Nit: Move to previous line.

::: netwerk/base/SimpleChannel.cpp:168
(Diff revision 2)
> +}
> +
> +void
> +SimpleChannelChild::AddIPDLReference()
> +{
> +  AddRef();

Nit: NS_ADDREF_THIS();

::: netwerk/base/SimpleChannel.cpp:169
(Diff revision 2)
> +
> +void
> +SimpleChannelChild::AddIPDLReference()
> +{
> +  AddRef();
> +  mIPCOpen = true;

I'd rather we just add a member `RefPtr<SimpleChannelChild> mIPDLRef` or something rather than a boolean with manual refcounting. Then just assign `this` to it in `ConnectParent`, and assert/null it out in `ActorDestroy`.

::: netwerk/base/SimpleChannel.cpp:177
(Diff revision 2)
> +void
> +SimpleChannelChild::ActorDestroy(ActorDestroyReason why)
> +{
> +  MOZ_ASSERT(mIPCOpen);
> +  mIPCOpen = false;
> +  Release();

Nit: NS_RELEASE_THIS();

::: netwerk/base/SimpleChannel.cpp:186
(Diff revision 2)
>  already_AddRefed<nsIChannel>
>  NS_NewSimpleChannelInternal(nsIURI* aURI, nsILoadInfo* aLoadInfo, UniquePtr<SimpleChannelCallbacks>&& aCallbacks)
>  {
> -  RefPtr<SimpleChannel> chan = new SimpleChannel(Move(aCallbacks));
> +  RefPtr<SimpleChannel> chan;
> +  if (IsNeckoChild()) {
> +    chan = new mozilla::net::SimpleChannelChild(Move(aCallbacks));

Nit: No need for `mozilla::net::`

::: netwerk/base/SimpleChannelParent.h:33
(Diff revision 2)
> +  NS_DECL_NSISTREAMLISTENER
> +
> +  MOZ_MUST_USE bool Init(const uint32_t& aArgs);
> +
> +private:
> +  ~SimpleChannelParent();

Nit: `~SimpleChannelParent() = default;`

::: netwerk/base/SimpleChannelParent.cpp:54
(Diff revision 2)
> +                                            const nsACString& aProvider,
> +                                            const nsACString& aPrefix)

Nit: Indentation.

::: netwerk/base/SimpleChannelParent.cpp:69
(Diff revision 2)
> +  // Nothing to do.
> +  return NS_OK;
> +}
> +
> +void
> +SimpleChannelParent::ActorDestroy(ActorDestroyReason why)

Nit: `aWhy`

::: netwerk/base/SimpleChannelParent.cpp:74
(Diff revision 2)
> +SimpleChannelParent::OnStartRequest(nsIRequest *aRequest,
> +                                  nsISupports *aContext)

Nit: Indentation, and `*` gets attached to the type, not the variable.

Same below.

::: netwerk/base/SimpleChannelParent.cpp:80
(Diff revision 2)
> +                                  nsISupports *aContext)
> +{
> +  // We don't have a way to prevent nsBaseChannel from calling AsyncOpen on
> +  // the created nsSimpleChannel. We don't have anywhere to send the data in the
> +  // parent, so abort the binding.
> +  return NS_BINDING_ABORTED;

Does this ever actually get called in practice? It seems like we should abort if it is.

::: netwerk/ipc/NeckoParent.h:190
(Diff revision 2)
> +  virtual PSimpleChannelParent*
> +    AllocPSimpleChannelParent(const uint32_t& channelId) override;
> +  virtual bool DeallocPSimpleChannelParent(PSimpleChannelParent* parent) override;
> +
> +  virtual mozilla::ipc::IPCResult RecvPSimpleChannelConstructor(PSimpleChannelParent* aActor,
> +                                                              const uint32_t& channelId) override;

Nit: Alignment. Also, please either use or don't use aArgs consistently.

::: netwerk/ipc/NeckoParent.cpp:544
(Diff revision 2)
> +}
> +
> +bool
> +NeckoParent::DeallocPSimpleChannelParent(PSimpleChannelParent* actor)
> +{
> +  RefPtr<SimpleChannelParent> p = dont_AddRef(static_cast<SimpleChannelParent*>(actor));

Nit: `dont_AddRef(actor).downcast<SimpleChannelParent>()`

::: netwerk/ipc/NeckoParent.cpp:550
(Diff revision 2)
> +  return true;
> +}
> +
> +mozilla::ipc::IPCResult
> +NeckoParent::RecvPSimpleChannelConstructor(PSimpleChannelParent* actor,
> +                                         const uint32_t& channelId)

Nit: Alignment, and previous comment re: aArgs.

::: netwerk/ipc/NeckoParent.cpp:553
(Diff revision 2)
> +mozilla::ipc::IPCResult
> +NeckoParent::RecvPSimpleChannelConstructor(PSimpleChannelParent* actor,
> +                                         const uint32_t& channelId)
> +{
> +  SimpleChannelParent* p = static_cast<SimpleChannelParent*>(actor);
> +  DebugOnly<bool> rv = p->Init(channelId);

Nit: Please call this `ok` or something, rather than `rv`. `rv` tends to imply `nsresult`.

But, even better, this should really be `MOZ_ALWAYS_TRUE(p->Init())` rather than a `DebugOnly` and an assert.

::: toolkit/modules/addons/WebRequest.jsm:837
(Diff revision 2)
> +      channel.QueryInterface(Ci.nsIJARChannel);
> +      return false;

Nit:

    if (channel instanceof Ci.nsIJARChannel) {
      return false;
    }

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

https://reviewboard.mozilla.org/r/156688/#review163776

::: browser/components/extensions/test/browser/browser-common.ini:94
(Diff revision 2)
> +[browser_ext_redirect.js]
> +[browser_ext_redirect_chrome.js]

Why are these browser tests? At most, they should probably be plain mochitests, but ideally they should probably be xpcshell.

::: browser/components/extensions/test/browser/browser_ext_redirect.js:68
(Diff revision 2)
> +    }
> +  });
> +  browser.tabs.create({url});
> +}
> +
> +// Tests whether we can redirect to a moz-extension: url.  This should result

Nit: "whether web content can redirect to a non-web-accessible moz-extension: URL"

::: browser/components/extensions/test/browser/browser_ext_redirect.js:81
(Diff revision 2)
> +  await extension.unload();
> +});
> +
> +// Tests whether we can redirect to a moz-extension: url.  This test adds the
> +// redirect destination to web_accessible_resources which should allow the
> +// redirect to occure.

*occur

::: browser/components/extensions/test/browser/browser_ext_redirect.js:92
(Diff revision 2)
> +  await extension.startup();
> +  await extension.awaitMessage("done");
> +  await extension.unload();
> +});
> +
> +// Tests whether we can redirect to a moz-extension: url.

Nit: "whether an extension can redirect ... using the webRequest API"

::: browser/components/extensions/test/browser/browser_ext_redirect_chrome.js:33
(Diff revision 2)
> +
> +function onModifyListener(originUrl, redirectToUrl) {
> +  let obs = Services.obs;
> +  return new Promise(resolve => {
> +    obs.addObserver({
> +      observe: function(subject, topic, data) {

Nit: Please use an observer function rather than an observer object. But, `TestUtils.topicObserved` would be even better.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/2-3/

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/2-3/

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

https://reviewboard.mozilla.org/r/156688/#review163776

> Why are these browser tests? At most, they should probably be plain mochitests, but ideally they should probably be xpcshell.

Moved into toolkit, and one into mochitest at least.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/3-4/

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/3-4/

Comment 22

[Shane Caraveo (:mixedpuppy)]

mayhemer: I've run into a wrinkle with redirection.  While rewriting the tests into xpcshell and using xhr rather than a browser for redirect testing, I noticed something not quite right.  There is potential the problem has to do with changes from bug 1334550, but it looks like it is in httpChannel.  

I'll try to explain what I'm seeing.

The tests do this:

Test 1:
1. initial xhr request is http://foo
2. redirect to moz-ext://bar via server 302

Test 2:
1. initial xhr request is http://foo
2. channel.redirectTo moz-ext://bar during http-on-modify-request

For both, I expect moz-ext://bar to be in either xhr.channel.URI or xhr.channel.originalURI in the xhr events (xhr.load, xhr.loadend, xhr.error)

What I get is the original http://foo in originalURI and a jar:file: in URI.


What is happening in the channel/protocol code:

AsyncOnChannelRedirect called from nsHttpChannel gets newChannel which has the values

  newChannel.URI -> jar:file:... (the translated path to moz-ext://bar)
  newChannel.originalURI -> moz-ext://bar

That seems correct.

nsHttpChannel::ContinueProcessRedirection is called (one code path, another is via OpenRedirectChannel)
  and mRedirectChannel->SetOriginalURI(mOriginalURI) is called
  mOriginalURI is http://foo

This means we end up with:

  channel.URI -> jar:file:...
  channel.originalURI -> http://foo

I think this is not what we want, it doesn't really make sense and isn't useful for me.

I would have actually expected either this (channel with internal translation to jar:file):

  channel.URI -> jar:file:...
  channel.originalURI -> moz-ext://bar

or this (channel showing redirection from originalURI to requested URI):

  channel.URI -> moz-ext://bar
  channel.originalURL -> http://foo


If I remove the SetOriginalURI calls that happen during redirection handling in nsHttpChannel, I end up with a channel that has data I can use to validate the redirect.  Another potential direction is to translate from the jar:file: uri back to a moz-ext uri if that is possible.

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> mayhemer: I've run into a wrinkle with redirection.  While rewriting the
> tests into xpcshell and using xhr rather than a browser for redirect
> testing, I noticed something not quite right.  There is potential the
> problem has to do with changes from bug 1334550, but it looks like it is in
> httpChannel.  
> 
> I'll try to explain what I'm seeing.
> 
> The tests do this:
> 
> Test 1:
> 1. initial xhr request is http://foo
> 2. redirect to moz-ext://bar via server 302
> 
> Test 2:
> 1. initial xhr request is http://foo
> 2. channel.redirectTo moz-ext://bar during http-on-modify-request
> 
> For both, I expect moz-ext://bar to be in either xhr.channel.URI or
> xhr.channel.originalURI in the xhr events (xhr.load, xhr.loadend, xhr.error)
> 
> What I get is the original http://foo in originalURI and a jar:file: in URI.
> 
> 
> What is happening in the channel/protocol code:
> 
> AsyncOnChannelRedirect called from nsHttpChannel gets newChannel which has
> the values
> 
>   newChannel.URI -> jar:file:... (the translated path to moz-ext://bar)
>   newChannel.originalURI -> moz-ext://bar
> 
> That seems correct.
> 
> nsHttpChannel::ContinueProcessRedirection is called (one code path, another
> is via OpenRedirectChannel)
>   and mRedirectChannel->SetOriginalURI(mOriginalURI) is called
>   mOriginalURI is http://foo
> 
> This means we end up with:
> 
>   channel.URI -> jar:file:...
>   channel.originalURI -> http://foo
> 
> I think this is not what we want, it doesn't really make sense and isn't
> useful for me.

No, this is EXACTLY what we want.  The original URI on an opened redirected channel points to the URI that was the first in the redirect chain, here the http://foo URI.  It's correct and expected.

> 
> I would have actually expected either this (channel with internal
> translation to jar:file):
> 
>   channel.URI -> jar:file:...
>   channel.originalURI -> moz-ext://bar

This is what you can see ONLY during the redirect veto checking, before the channel is finally asyncOpen()'ed.

> 
> or this (channel showing redirection from originalURI to requested URI):
> 
>   channel.URI -> moz-ext://bar
>   channel.originalURL -> http://foo
> 
> 
> If I remove the SetOriginalURI calls that happen during redirection handling
> in nsHttpChannel, I end up with a channel that has data I can use to
> validate the redirect.  Another potential direction is to translate from the
> jar:file: uri back to a moz-ext uri if that is possible.

Use the result principal (final channel) URL to resolve it.  It's set to the moz-ext:// URI.  In C use NS_GetFinalChannelURI or when you are in js, write an equivalent of:

finalChannelURI = channel->?loadInfo->resultPrincipalURI || channel->originalURI

This is exactly what you want to use.  Before we change channel->originalURI in nsHttpChannel::ContinueProcessRedirection we store the channel's original (or better said 'creation') URL in loadInfo->resultPrincipalURI, if the handler hasn't change it during the channel creation (in moz-ext case it leaves it unchanged, which is right!)  This was the whole effort in bug 1319111 :)

Comment 24

[Haik Aftandilian [:haik]]

Clearing my needinfo in light of Honza's reply.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/4-5/

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/4-5/

Comment 27

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

re-requesting reviews specifically for changes:

in WebRequest.jsm where I am using resultPrincipalURI as a part of getting the URI used for WebRequest listeners

in the test where I've commented about a problem in the xhr.load event handler.

mayhemer, if you can look at those particular items from your perspective, kris can handle the rest of the details.

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

https://reviewboard.mozilla.org/r/156690/#review165208

::: netwerk/base/SimpleChannel.cpp:110
(Diff revision 5)
> +private:
> +  virtual ~SimpleChannelChild() = default;
> +
> +  void AddIPDLReference();
> +
> +  RefPtr<SimpleChannelChild> mIPDLRef;

liking this :)

::: netwerk/base/SimpleChannelParent.cpp:73
(Diff revision 5)
> +
> +NS_IMETHODIMP
> +SimpleChannelParent::OnStartRequest(nsIRequest* aRequest,
> +                                    nsISupports* aContext)
> +{
> +  // We don't have a way to prevent nsBaseChannel from calling AsyncOpen on

prevent nsHttpChannel

::: toolkit/modules/addons/WebRequest.jsm:61
(Diff revision 5)
>  }
>  
> +function getFinalChannelURI(channel) {
> +  let {loadInfo} = channel;
> +  let finalChannelURI = loadInfo ? loadInfo.resultPrincipalURI : channel.originalURI;
> +  return finalChannelURI || channel.URI;

have you ever encountered a case when both loadInfo.resultPrincipalURI and channel.originalURI were null?  if so, then it's likely a bug.  originalURI must always be set on a channel, hence return || channel.URI is IMO misleading.

::: toolkit/modules/addons/WebRequest.jsm:1070
(Diff revision 5)
>    },
>  
>    onChannelReplaced(oldChannel, newChannel) {
> +    // We want originalURI if available, this will provide a moz-ext rather than
> +    // jar or file uri on redirects.
> +    let uri = newChannel.originalURI || newChannel.URI;

hmm... when exactly is this callback called?  I presume before the new channel is opened?  If so, then this is correct. 

The same note about originalURI applies here.

Comment 29

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #14)
> Comment on attachment 8885911 [details]

> ::: netwerk/ipc/NeckoParent.cpp:544
> (Diff revision 2)
> > +}
> > +
> > +bool
> > +NeckoParent::DeallocPSimpleChannelParent(PSimpleChannelParent* actor)
> > +{
> > +  RefPtr<SimpleChannelParent> p = dont_AddRef(static_cast<SimpleChannelParent*>(actor));
> 
> Nit: `dont_AddRef(actor).downcast<SimpleChannelParent>()`
> 

This causes a build failure on osx cross compile.

https://treeherder.mozilla.org/logviewer.html#?job_id=116460736&repo=try&lineNumber=10874

Builds fine on my local osx laptop.

Comment 30

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/5-6/

Comment 31

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/5-6/

Comment 32

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> (In reply to Kris Maglione [:kmag] from comment #14)
> > > +  RefPtr<SimpleChannelParent> p = dont_AddRef(static_cast<SimpleChannelParent*>(actor));
> > 
> > Nit: `dont_AddRef(actor).downcast<SimpleChannelParent>()`
> > 
> 
> This causes a build failure on osx cross compile.

To be clear, I meant:

    RefPtr<SimpleChannelParent> p = dont_AddRef(actor).downcast<SimpleChannelParent>();

You still need to assign to a RefPtr so that the original ref is dropped. It causes a build failure on OS-X because we have the clang static analysis plugin enabled there, which catches things like failure to use must-use types. Even if it didn't, though, this would cause debug test failures because of leaks.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/6-7/

Comment 34

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/6-7/

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

https://reviewboard.mozilla.org/r/156688/#review166024

We still need to test requests that originate in the content process, which this test doesn't appear to do. Your original approach was good, just use `ExtensionTestUtils.loadContentPage(...)` rather than the tabs API.

::: toolkit/components/extensions/test/xpcshell/test_ext_redirects.js:14
(Diff revision 7)
> +let gServerUrl;
> +function startHttpServer() {
> +  let httpServer = new HttpServer();
> +  httpServer.start(-1);
> +  httpServer.registerDirectory("/", do_get_cwd());
> +  httpServer.registerContentType("sjs", "sjs");
> +  gServerUrl = "http://localhost:" + httpServer.identity.primaryPort;
> +  do_register_cleanup(async function cleanup_httpServer() {
> +    await new Promise(resolve => {
> +      httpServer.stop(resolve);
> +    });
> +  });
> +  return httpServer;
> +}

No need for an sjs script. Just use `createHttpServer()` and `registerPathHandler(...)`. See test_ext_downloads_download.js for an example.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/7-8/

Comment 37

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/7-8/

Comment 38

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #14)
> Comment on attachment 8885911 [details]
> Bug 1380186 implement SimpleChannel Parent/Child IPC,
> 
> https://reviewboard.mozilla.org/r/156690/#review163724

> ::: netwerk/base/SimpleChannelParent.cpp:80
> (Diff revision 2)
> > +                                  nsISupports *aContext)
> > +{
> > +  // We don't have a way to prevent nsBaseChannel from calling AsyncOpen on
> > +  // the created nsSimpleChannel. We don't have anywhere to send the data in the
> > +  // parent, so abort the binding.
> > +  return NS_BINDING_ABORTED;
> 
> Does this ever actually get called in practice? It seems like we should
> abort if it is.

This does in fact end up getting called, at least on all redirects.  The redirected channel succeeds on the request, but passes NS_BINDING_ABORTED in a call to onStop.  This (I think) sets channel.statusCode to NS_BINDING_ABORTED, which ultimately results in onErrorOccurred to be called if you're listening on that with webRequest.

The patches just submitted include two additional tests that are currently skipped.  They produce the failure I'm seeing.  The content browser itself does get the expected result uri in browser.documentURI, but we also get the onErrorOccurred call.

Comment 39

[Shane Caraveo (:mixedpuppy)]

Just a little tracing I did with one of the skipped tests:

 0:05.63 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** runChannelListener onRedirect [object Object]"
 0:05.64 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** runChannelListener onStart null"
 0:05.64 PROCESS_OUTPUT: Thread-1 (pid:37366) "SimpleChannelParent::OnStartRequest called, NS_BINDING_ABORTED"
 0:05.64 PROCESS_OUTPUT: Thread-1 (pid:37366) "SimpleChannelParent::OnStopRequest called"
 0:05.64 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** onStopRequest statusCode 2152398850"
 0:05.64 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** runChannelListener onStop null"
 0:05.65 PROCESS_OUTPUT: Thread-1 (pid:37366) "***maybeError {"error":"NS_BINDING_ABORTED"} 2152398850"
 0:05.65 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** run onError now"
 0:05.65 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** runChannelListener onError [object Object]"
 0:05.65 PROCESS_OUTPUT: Thread-1 (pid:37366) "*** runChannelListener errorCheck exit onStop"
 0:05.65 TEST_STATUS: Thread-1 test_extension_302_redirect PASS [test_extension_302_redirect : 238] redirect matches - Expected: moz-extension://dc9c86e6-2d56-644c-b2f0-f2780079a993/finished.html, Actual: moz-extension://dc9c86e6-2d56-644c-b2f0-f2780079a993/finished.html - true == true
 0:05.65 LOG: Thread-1 INFO "onErrorOccurred {"requestId":"2","url":"moz-extension://dc9c86e6-2d56-644c-b2f0-f2780079a993/finished.html","tabId":-1,"type":"main_frame","timeStamp":1501029456035,"frameId":0,"parentFrameId":-1,"fromCache":false,"error":"NS_BINDING_ABORTED"}"
 0:05.66 TEST_STATUS: Thread-1 test_extension_302_redirect FAIL [test_extension_302_redirect : 250] requestCompleted - false == true

Comment 40

[Kris Maglione [:kmag]]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

https://reviewboard.mozilla.org/r/156688/#review167026

::: toolkit/components/extensions/test/xpcshell/redirect.sjs:1
(Diff revision 8)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +Components.utils.importGlobalProperties(["URLSearchParams"]);
> +
> +function handleRequest(request, response) {
> +  let params = new URLSearchParams(request.queryString);
> +  if (params.has("no_redirect")) {
> +    response.setStatusLine(request.httpVersion, 200, "OK");
> +    response.write("ok");
> +  } else {
> +    response.setStatusLine(request.httpVersion, 302, "Moved Temporarily");
> +    response.setHeader("Location", params.get("redirect_uri"));
> +  }
> +}

Please delete this file.

::: toolkit/components/extensions/test/xpcshell/test_ext_redirects.js:54
(Diff revision 8)
> +  return TestUtils.topicObserved("http-on-modify-request", (subject, data) => {
> +    let channel = subject.QueryInterface(Ci.nsIHttpChannel);
> +    return channel.URI && channel.URI.spec == originUrl;
> +  }).then(([subject, data]) => {
> +    let channel = subject.QueryInterface(Ci.nsIHttpChannel);
> +    channel.suspend();

Why suspend?

::: toolkit/components/extensions/test/xpcshell/xpcshell.ini:10
(Diff revision 8)
>  dupe-manifest =
>  support-files =
>    data/**
>    head_sync.js
>    xpcshell-content.ini
> +  redirect.sjs

Please delete this file.

Comment 41

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885910 [details]
Bug 1380186 test http redirects to moz-ext protocol,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156688/diff/8-9/

Comment 42

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8885911 [details]
Bug 1380186 implement SimpleChannel Parent/Child IPC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156690/diff/8-9/

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73f48b4bd601
test http redirects to moz-ext protocol, r=kmag
https://hg.mozilla.org/integration/autoland/rev/21996e01ca43
implement SimpleChannel Parent/Child IPC, r=kmag,mayhemer

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73f48b4bd601
https://hg.mozilla.org/mozilla-central/rev/21996e01ca43

Comment 45

[Dietrich Ayala (:dietrich)]

I have a report that this feature no longer works, possibly regressed. Filed in bug 1416146.

Comment 46

[Kris Maglione [:kmag]]

(In reply to Dietrich Ayala (:dietrich) from comment #45)
> I have a report that this feature no longer works, possibly regressed. Filed
> in bug 1416146.

It's tested, and the tests still pass, so that's unlikely.