SimpleChannel needs Parent/Child IPC

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
P1
normal
RESOLVED FIXED
a month ago
4 days ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Blocks: 2 bugs)

49 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a090485a6aa4b370c5151d01e223d706de87b02
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8885904 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
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.
Attachment #8885911 - Flags: feedback?(kmaglione+bmo)
Attachment #8885911 - Flags: feedback?(honzab.moz)
(Assignee)

Updated

a month ago
No longer blocks: 1276048
Duplicate of this bug: 1276048
(Assignee)

Updated

a month ago
webextensions: --- → +
(Assignee)

Updated

a month ago
Depends on: 1367814
(Assignee)

Updated

a month ago
Blocks: 1213483
(Assignee)

Updated

a month ago
Duplicate of this bug: 1256122
(Assignee)

Updated

a month ago
See Also: → bug 1368239
(Assignee)

Comment 9

a month ago
maybe tests will run this time.

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

Comment 10

a month ago
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
Attachment #8885911 - Flags: review?(kmaglione+bmo)
Attachment #8885911 - Flags: review?(honzab.moz)
Attachment #8885911 - Flags: feedback?(kmaglione+bmo)
Attachment #8885911 - Flags: feedback?(honzab.moz)
(Assignee)

Updated

a month ago
Attachment #8885910 - Flags: review?(kmaglione+bmo)

Comment 11

a month ago
mozreview-review
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
Attachment #8885911 - Flags: review?(honzab.moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a month ago
mozreview-review
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;
    }
Attachment #8885911 - Flags: review?(kmaglione+bmo) → review+

Comment 15

a month ago
mozreview-review
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.
Attachment #8885910 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a month ago
Blocks: 1382007
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a month ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Duplicate of this bug: 1380693
(Assignee)

Comment 22

a month ago
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.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(haftandilian)
(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 :)
Flags: needinfo?(honzab.moz)

Updated

29 days ago
No longer blocks: 1380156
Clearing my needinfo in light of Honza's reply.
Flags: needinfo?(haftandilian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

29 days ago
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.
Attachment #8885911 - Flags: review?(kmaglione+bmo)
Attachment #8885911 - Flags: review?(honzab.moz)
Attachment #8885911 - Flags: review+

Comment 28

28 days ago
mozreview-review
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.
Attachment #8885911 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 29

28 days ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

25 days ago
mozreview-review
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.
Attachment #8885910 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 38

24 days ago
(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.
(Assignee)

Comment 39

24 days ago
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

23 days ago
mozreview-review
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.
Attachment #8885910 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

22 days ago
Pushed by mixedpuppy@gmail.com:
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

22 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73f48b4bd601
https://hg.mozilla.org/mozilla-central/rev/21996e01ca43
Status: NEW → RESOLVED
Last Resolved: 22 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

4 days ago
See Also: → bug 1390346
You need to log in before you can comment on or make changes to this bug.