Closed Bug 1331730 Opened 7 years ago Closed 7 years ago

[e10s] CORS error messages (at least involving preflights) don't show up in the web console with e10s

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox53 --- affected
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: kmckinley)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

STEPS TO REPRODUCE:

1)  Back out the fix for bug 1328761 (but not the tests).
2)  Run the test.
3)  Observe it fail.
4)  Look at the web console.  Observe no CORS error message there.
5)  Rerun in non-e10s mode, observer error message in web console.

The message _does_ appear in the browser console when running in e10s mode.  So the problem, presumably, is that we don't get a useful window id from the preflight channel or something... In any case, this lack of useful error messages in the web console for this case is pretty bad, because web developers have no idea the browser console exists.

Christoph, do you know who should own this?
Flags: needinfo?(ckerschb)
Most likely the problem can be localized somewhere around here [1] because for some reason we can't query the right windowId. We had a simliar problem with CSP once, which we fixed here [2].

Kate, any chance I could convince you to take a look?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#103
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#658
Flags: needinfo?(ckerschb) → needinfo?(kmckinley)
HSTS priming has the same problem, when we can resolve the decision we are in the parent and don't have the correct windowId. In HSTS priming case, we need to hold off on updating the UI as mixed-content until after the decision is made.
Flags: needinfo?(kmckinley)
(In reply to Kate McKinley [:kmckinley] from comment #2)
> HSTS priming has the same problem, when we can resolve the decision we are
> in the parent and don't have the correct windowId. In HSTS priming case, we
> need to hold off on updating the UI as mixed-content until after the
> decision is made.

The loadInfo should hold the right information though, probably we should update to use that info.

windowid = channel->loadInfo->innerWindowID;
Assignee: nobody → kmckinley
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
I feel pretty strongly that this is something we should prioritize, because silent failure with no indication of why is the sort of thing that will drive away web developers _very_ fast...

Is the problem here just lack of someone with time to work on this?  If so, I can try to solve that problem...
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> I feel pretty strongly that this is something we should prioritize, because
> silent failure with no indication of why is the sort of thing that will
> drive away web developers _very_ fast...

Convinced. Kate, any chance you can tackle this within this cycle? Otherwise please let me know and I'll have a look.
Flags: needinfo?(ckerschb) → needinfo?(kmckinley)
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Status: NEW → ASSIGNED
Flags: needinfo?(kmckinley)
From the simple hand testing I've been able to do so far, this patch does the right thing. Still needs tests. I think they have to be browser mochitests so we have access to the web console logs, and the existing tests are plain mochitest.
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

https://reviewboard.mozilla.org/r/112994/#review114548

::: netwerk/protocol/http/HttpBaseChannel.cpp:3801
(Diff revision 1)
>    }
>    aConnectionInfoHashKey.Assign(mConnectionInfo->HashKey());
>    return NS_OK;
>  }
>  
> +

Please take this extra line back out.

::: netwerk/protocol/http/HttpChannelChild.cpp:2980
(Diff revision 1)
> +  uint64_t innerWindowID = 0;
> +  Unused << mLoadInfo->GetInnerWindowID(&innerWindowID);

Just do:

  uint64_t innerWindowID = mLoadInfo->GetInnerWindowID();

::: netwerk/protocol/http/nsCORSListenerProxy.h:74
(Diff revision 1)
>  
>    void SetInterceptController(nsINetworkInterceptController* aInterceptController);
>  
> +  static void LogBlockedCORSRequest(uint64_t aInnerWindowID,
> +                                    const nsAString& aMessage);
> +  

Please remove this whitespace-only line too.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:91
(Diff revision 1)
>  
>    nsAutoString msg(blockedMessage.get());
>  
> -  // query innerWindowID and log to web console, otherwise log to
> -  // the error to the browser console.
> -  uint64_t innerWindowID = nsContentUtils::GetInnerWindowID(aRequest);
> +  if (XRE_IsParentProcess()) {
> +    if (aParentChannel) {
> +      rv = aParentChannel->LogBlockedCORSRequest(msg);

Can we not propagate the right inner window id to the parent channel so this would all Just Work without the manual proxying?  Or would that be meaningless in the parent process?

Anyway, if we can't do that, then this patch looks good.

::: netwerk/protocol/http/nsIHttpChannel.idl:487
(Diff revision 1)
> +     * need to proxy the request to the child.
> +     *
> +     * @param aMessage
> +     *        The message to print in the console.
> +     */
> +    void LogBlockedCORSRequest(in AString aMessage);

"logBlockedCORSRequest".
Attachment #8837981 - Flags: review?(bzbarsky) → review+
Comment on attachment 8839224 [details] [diff] [review]
(for splinter review) Bug 1331730 - Log CORS messages from the content process

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

Before I review this patch I need (I'm asking for this so often...) some background of what the patch does, its good description.  Can you provide one please?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3797,5 @@
>    aConnectionInfoHashKey.Assign(mConnectionInfo->HashKey());
>    return NS_OK;
>  }
>  
> +

extra space

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1849,5 @@
> +  if (mIPCClosed ||
> +      NS_WARN_IF(!SendLogBlockedCORSRequest(nsString(aMessage)))) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  return NS_OK; //;

//?

@@ +1850,5 @@
> +      NS_WARN_IF(!SendLogBlockedCORSRequest(nsString(aMessage)))) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  return NS_OK; //;
> +}

add a blank line after this line please

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +168,5 @@
>    // Tell the child to issue a deprecation warning.
>    async IssueDeprecationWarning(uint32_t warning, bool asError);
>  
> +  // When CORS blocks in the parent process, it doesn't have the correct
> +  // window ID, so send it back to the child.

"send it back" - what exactly does "it" stand for?  please update the comment so there are no possible ambiguities

::: netwerk/protocol/http/nsCORSListenerProxy.h
@@ +70,5 @@
>    void SetInterceptController(nsINetworkInterceptController* aInterceptController);
>  
> +  static void LogBlockedCORSRequest(uint64_t aInnerWindowID,
> +                                    const nsAString& aMessage);
> +  

extra ws
please add a comment what this does, when called.

@@ +102,5 @@
>    nsCOMPtr<nsIInterfaceRequestor> mOuterNotificationCallbacks;
>    nsCOMPtr<nsINetworkInterceptController> mInterceptController;
> +  // This is needed so that a reference can work its way back to the child, if
> +  // needed
> +  nsCOMPtr<nsIHttpChannel> mHttpChannel;

please push to try and keep an eye for leaks.  add a comment this is released in OnStopRequest.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +52,5 @@
>  
>  static bool gDisableCORS = false;
>  static bool gDisableCORSPrivateData = false;
>  
> +

extra space

@@ +95,1 @@
>  

extra space

@@ +103,3 @@
>  }
>  
> +

extra space

@@ +948,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aChannel);
>    NS_ENSURE_TRUE(http, NS_ERROR_FAILURE);
> +  mHttpChannel = http;

add a blank line before this line please

@@ +1081,5 @@
>    nsCOMPtr<nsIPrincipal> mReferrerPrincipal;
>    nsCOMPtr<nsICorsPreflightCallback> mCallback;
>    nsCOMPtr<nsILoadContext> mLoadContext;
>    bool mWithCredentials;
> +

you are a space junkie :D

@@ +1465,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  /**
> +   * TODO
> +   * Add the original channel parent's warning reporter to preflight channel
> +   */

do we have a followup for this?

@@ +1517,5 @@
>  
>    return NS_OK;
>  }
> +
> +

and an extra space

@@ +1519,5 @@
>  }
> +
> +
> +void
> +nsCORSListenerProxy::LogBlockedCORSRequest(uint64_t aInnerWindowID,

if this is static please add // static comment
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

https://reviewboard.mozilla.org/r/112994/#review115452

feedback in comment 10
Comment 10 - first paragraph please.  Thanks.
Flags: needinfo?(kmckinley)
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

Dropping r until the ni is answered.
Attachment #8837981 - Flags: review?(honzab.moz)
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

https://reviewboard.mozilla.org/r/112994/#review120968

r- before I get the necessary info.  Thanks.
Attachment #8837981 - Flags: review-
Apologies for the long delay.

The current patch addresses all the feedback from bz and mayhemer above. As well, it adds a browser mochitest to ensure that messages are printed to the console. There are a couple issues that need to be addressed.

1) The test times out on some (linux mostly) systems on try. It does a lot, so I suspect requestLongerTimeout() will be sufficient.
2) This patch redirects most, but not all, messages to the web console. In some cases, the parent channel appears to be unable to log the request. Which is related to the next issue.
3) The test produces inconsistent results with the number of messages. On my system, I see 50 messages, but on try I also see 60, and 47. The differences seem to be consistent.

try run that reflects issue #3 (based on a slightly older version of the test)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dadb70e257937c6458cb9cc51efa593e7230caf&selectedJob=87707691

Should this be broken out into another bug, or I'm open to suggestions on where to look as I've had no luck so far.
Flags: needinfo?(kmckinley)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(bzbarsky)
Variable numbers like that generally indicate some sort of race in the test...

Looking at the test in the linked try commit, there's a SpecialPowers.pushPrefEnv call (an async operation) that is immediately followed by code that does stuff.  Should the test be waiting for the preference change to actually happen before proceeding?  Does making it do so change things to be consistent?
Flags: needinfo?(bzbarsky)
Kate, I'm still missing description of the patch - what it fixes and how.  Can you write one or direct me to a comment where I can find that out easily?  Thanks.
Flags: needinfo?(kmckinley)
Attachment #8839224 - Attachment is obsolete: true
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #18)
> Kate, I'm still missing description of the patch - what it fixes and how. 
> Can you write one or direct me to a comment where I can find that out
> easily?  Thanks.

Is it this?

"In e10s, a channel created by parent does not have a reliable reference
to the inner window ID that initiated the request. Without that, the
channel must request that the content process log and blocked messages
to the web console. This patch creates a new ipdl interface to pass the
message from the parent to the child process. The nsCORSListenerProxy
also needs to keep a reference to the nsIHttpChannel that created it so
it can find its way back to the child."

Well hidden in the reviewboard...
Flags: needinfo?(kmckinley)
(In reply to Kate McKinley [:kmckinley] from comment #16)
> Apologies for the long delay.
> 
> The current patch addresses all the feedback from bz and mayhemer above. As
> well, it adds a browser mochitest to ensure that messages are printed to the
> console. There are a couple issues that need to be addressed.
> 
> 1) The test times out on some (linux mostly) systems on try. It does a lot,
> so I suspect requestLongerTimeout() will be sufficient.

Your new test seems similar to what other huge CORS tests are performing.  Maybe use the same timeout value as they use?

> 2) This patch redirects most, but not all, messages to the web console. In
> some cases, the parent channel appears to be unable to log the request.

Maybe add printf or logs to the patch where the code branches and push to try?

> Which is related to the next issue.
> 3) The test produces inconsistent results with the number of messages. On my
> system, I see 50 messages, but on try I also see 60, and 47. The differences
> seem to be consistent.

Have you compared the message lists it prints?  It could hint what's wrong.

> 
> try run that reflects issue #3 (based on a slightly older version of the
> test)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6dadb70e257937c6458cb9cc51efa593e7230caf&selectedJob=8
> 7707691
> 
> Should this be broken out into another bug, or I'm open to suggestions on
> where to look as I've had no luck so far.

I think everything should be logged as this bug is just about that.
Comment on attachment 8855826 [details] [diff] [review]
(For splinter review) Bug 1331730 - Log CORS messages from the content process

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

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +86,5 @@
>    nsAutoString msg(blockedMessage.get());
>  
> +  if (XRE_IsParentProcess()) {
> +    if (aParentChannel) {
> +      rv = aParentChannel->LogBlockedCORSRequest(msg);

are you sure it's HttpChannelParent class here all the time when we are on the parent process and this is for a child originated load?

I checked in a debugger and it isn't.

@@ +403,5 @@
>      mOriginHeaderPrincipal(aRequestingPrincipal),
>      mWithCredentials(aWithCredentials && !gDisableCORSPrivateData),
>      mRequestApproved(false),
> +    mHasBeenCrossSite(false),
> +    mHttpChannel(nullptr)

no need to init a comptr.

@@ +947,5 @@
>    nsAutoCString origin;
>    rv = nsContentUtils::GetASCIIOrigin(mOriginHeaderPrincipal, origin);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr <nsIHttpChannel> http = do_QueryInterface(aChannel);

excessive space

@@ +1290,5 @@
>  
>    bool succeedded;
>    rv = http->GetRequestSucceeded(&succeedded);
>    if (NS_FAILED(rv) || !succeedded) {
> +    LogBlockedRequest(aRequest, "CORSPreflightDidNotSucceed", nullptr, parentHttpChannel);

here the parent channel is taken from mCallback...

@@ +1382,5 @@
>    *aPreflightChannel = nullptr;
>  
>    if (gDisableCORS) {
> +    nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aRequestChannel);
> +    LogBlockedRequest(aRequestChannel, "CORSDisabled", nullptr, http);

and here you query the request channel as the channel to use for logging.
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

https://reviewboard.mozilla.org/r/112994/#review128982

https://bugzilla.mozilla.org/show_bug.cgi?id=1331730#c22

::: dom/security/test/cors/file_cors_logging_test.html:1
(Diff revision 2)
> +<!DOCTYPE HTML>

is this a copy of an existing file?  if so, could you use 'hg cp' instead?
Attachment #8837981 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #22)
> Comment on attachment 8855826 [details] [diff] [review]
> (For splinter review) Bug 1331730 - Log CORS messages from the content
> process
> 
> Review of attachment 8855826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> @@ +86,5 @@
> >    nsAutoString msg(blockedMessage.get());
> >  
> > +  if (XRE_IsParentProcess()) {
> > +    if (aParentChannel) {
> > +      rv = aParentChannel->LogBlockedCORSRequest(msg);
> 
> are you sure it's HttpChannelParent class here all the time when we are on
> the parent process and this is for a child originated load?
> 
> I checked in a debugger and it isn't.

It should not be the HttpChannelParent, but the nsIHttpChannel that created this. I'll change the argument name to make that more clear.

> 
> @@ +1290,5 @@
> >  
> >    bool succeedded;
> >    rv = http->GetRequestSucceeded(&succeedded);
> >    if (NS_FAILED(rv) || !succeedded) {
> > +    LogBlockedRequest(aRequest, "CORSPreflightDidNotSucceed", nullptr, parentHttpChannel);
> 
> here the parent channel is taken from mCallback...
> 
> @@ +1382,5 @@
> >    *aPreflightChannel = nullptr;
> >  
> >    if (gDisableCORS) {
> > +    nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aRequestChannel);
> > +    LogBlockedRequest(aRequestChannel, "CORSDisabled", nullptr, http);
> 
> and here you query the request channel as the channel to use for logging.

The second case is in a static method, so there is no mCallback.
(In reply to Honza Bambas (:mayhemer) from comment #23)
> Comment on attachment 8837981 [details]
> Bug 1331730 - Log CORS messages from the content process
> 
> https://reviewboard.mozilla.org/r/112994/#review128982
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1331730#c22
> 
> ::: dom/security/test/cors/file_cors_logging_test.html:1
> (Diff revision 2)
> > +<!DOCTYPE HTML>
> 
> is this a copy of an existing file?  if so, could you use 'hg cp' instead?

I am using git-cinnabar, and there is no git cp. I'll see what I can do.

In words, yes, this file is a copy of an existing test_CrossSiteXHR.html, with all of the tests deleted. The is/ok methods used don't appear to work in browser mochitests when loaded like this. Since the browser_CORS-console-warnings.js actually looks at the messages logged, this should be fine.
TBH, I switched from Nightly to Chrome just because of this. It's very hard to develop ajaxy websites - or just websites embedding external ressources like fonts or maps - when CORS errors are silently dropped :-/

Looks like this bug has stalled 3 months ago... Kate, are you still working on this?
Flags: needinfo?(kmckinley)
Flags: needinfo?(honzab.moz)
Boris, what is the question?
Can you please coordinate with Kate to make sure this gets finished up and landed?
I think what needs to happen in order to get 100% logging coverage is to propagate the HttpChannelParent to any nsHttpChannel instance which are created for the CORS preflight. However, there are still 10 instances where the nsHttpChannel is in the parent and can not find the warning reporter. I will see if I can categorize those tomorrow.
Flags: needinfo?(kmckinley)
Looks like things started to move.  I'll get to the review soon.
Flags: needinfo?(honzab.moz)
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

https://reviewboard.mozilla.org/r/112994/#review162900

still few details to update, but after some local testing and adjustments I can say this more or less works.  we just need to figure out the inconsistencies in number of messages.

r-, I'd like to see an update patch.

::: dom/security/test/cors/browser_CORS-console-warnings.js:42
(Diff revision 3)
> + */
> +function setCookiePref() {
> +  return new Promise(resolve =>
> +    SpecialPowers.pushPrefEnv({
> +      set: [
> +        ["network.cookie.cookieBehavior", 0],

add a comment why you are doing this.

::: dom/security/test/cors/browser_CORS-console-warnings.js:60
(Diff revision 3)
> +}
> +
> +//jscs:disable
> +add_task(function*() {
> +  //jscs:enable
> +  requestLongerTimeout(4);

comment why you need this

::: netwerk/base/nsILoadInfo.idl:776
(Diff revision 3)
> +
> +  /**
> +   * true in the parent if the child initiated this load. (only valid in the
> +   * parent)
> +   */
> +  [noscript] attribute boolean childInitiated;

please don't add this attr, set on the channel directly as suggested below.

::: netwerk/protocol/http/HttpChannelParent.cpp:508
(Diff revision 3)
>  
>    if (!loadInfo) {
>      return SendFailedAsyncOpen(NS_ERROR_UNEXPECTED);
>    }
>  
> +  Unused << loadInfo->SetChildInitiated(true);

please set this as a property on the underlying nsHttpChannel, you always have access if you need it.  Tho, I'm not much in favor or exposing this information to the channel, we so far lived well w/o it.  

You will have to do that also when redirected-to in HttpChannelParent::ConnectChannel as well as assigning the reporter:

@@ -802,16 +802,18 @@ HttpChannelParent::ConnectChannel(const
   LOG(("  found channel %p, rv=%08" PRIx32, channel.get(), static_cast<uint32_t>(rv)));
   mChannel = do_QueryObject(channel);
   if (!mChannel) {
     LOG(("  but it's not nsHttpChannel"));
     Delete();
     return true;
   }

+  mChannel->SetWarningReporter(this);
+


w/o it, I've run the test from bug 1328761 with the code changes from that bug commented out (hopefully correctly) and saw a lot of cases where mWarningReporter was not assigned on the channel, making nsHttpChannel::LogBlockedCORSRequest fail with unexpected.

There is also some inconsistency (not necessarily caused by this patch) when these changes are there:
- 4 tests are failing for me
- there is one cors failure error message when I open the web console after the test has finished
- there are four (10% cases) or five (90% cases) cors error messages when I open the console, clear it and rerun the test (ctrl-f5 or just f5)

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:1381
(Diff revision 3)
>      sPreflightCache->RemoveEntries(aURI, aRequestingPrincipal);
>    }
>  }
>  
>  nsresult
>  nsCORSListenerProxy::StartCORSPreflight(nsIChannel* aRequestChannel,

when you are here, please add // static comment before the method definition.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:1486
(Diff revision 3)
>    rv = preHttp->
>      SetRequestHeader(NS_LITERAL_CSTRING("Access-Control-Request-Method"),
>                       method, false);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  nsHttpChannel* reqCh = static_cast<nsHttpChannel*>(aRequestChannel);
> +  nsHttpChannel* preCh = static_cast<nsHttpChannel*>(preHttp.get());

use do_QueryObject instead.

::: netwerk/protocol/http/nsHttpChannel.h:196
(Diff revision 3)
>      AddSecurityMessage(const nsAString& aMessageTag,
>                         const nsAString& aMessageCategory) override;
> +    NS_IMETHOD LogBlockedCORSRequest(const nsAString& aMessage) override;
>  
> -    void SetWarningReporter(HttpChannelSecurityWarningReporter* aReporter)
> -      { mWarningReporter = aReporter; }
> +    void SetWarningReporter(HttpChannelSecurityWarningReporter *aReporter);
> +      //{ mWarningReporter = aReporter; }

remove these please

::: netwerk/protocol/http/nsHttpChannel.cpp:434
(Diff revision 3)
> +                (XRE_IsParentProcess()) ? "parent" : "child",
> +                (XRE_IsParentProcess()) ? ((fromChild) ? "child" : "parent") : "invalid"));
> +    if (mWarningReporter) {
> +        return mWarningReporter->LogBlockedCORSRequest(aMessage);
> +    }
> +    NS_WARNING("nsHttpChannel does not have a warning reporter");

are there legal cases when the missing warn reporter can be null, or is this really 100% legit?  Just want to avoid another annoying warning in the console.

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp:1051
(Diff revision 3)
>  }
> +
> +NS_IMETHODIMP
> +nsViewSourceChannel::LogBlockedCORSRequest(const nsAString& aMessage)
> +{
> +  return mHttpChannel->LogBlockedCORSRequest(aMessage);

null check please (+ NS_WARNING probably?)
Attachment #8837981 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #32)
> Comment on attachment 8837981 [details]
> Bug 1331730 - Log CORS messages from the content process
> 
> https://reviewboard.mozilla.org/r/112994/#review162900
> 
> still few details to update, but after some local testing and adjustments I
> can say this more or less works.  we just need to figure out the
> inconsistencies in number of messages.
> 
> r-, I'd like to see an update patch.
> 
> ::: dom/security/test/cors/browser_CORS-console-warnings.js:42
> (Diff revision 3)
> > + */
> > +function setCookiePref() {
> > +  return new Promise(resolve =>
> > +    SpecialPowers.pushPrefEnv({
> > +      set: [
> > +        ["network.cookie.cookieBehavior", 0],
> 
> add a comment why you are doing this.
> 
> ::: dom/security/test/cors/browser_CORS-console-warnings.js:60
> (Diff revision 3)
> > +}
> > +
> > +//jscs:disable
> > +add_task(function*() {
> > +  //jscs:enable
> > +  requestLongerTimeout(4);
> 
> comment why you need this
> 
> ::: netwerk/base/nsILoadInfo.idl:776
> (Diff revision 3)
> > +
> > +  /**
> > +   * true in the parent if the child initiated this load. (only valid in the
> > +   * parent)
> > +   */
> > +  [noscript] attribute boolean childInitiated;
> 
> please don't add this attr, set on the channel directly as suggested below.

This won't be in the final version, this was added to test a hypothesis.

> 
> You will have to do that also when redirected-to in
> HttpChannelParent::ConnectChannel as well as assigning the reporter:
> 
> @@ -802,16 +802,18 @@ HttpChannelParent::ConnectChannel(const
>    LOG(("  found channel %p, rv=%08" PRIx32, channel.get(),
> static_cast<uint32_t>(rv)));
>    mChannel = do_QueryObject(channel);
>    if (!mChannel) {
>      LOG(("  but it's not nsHttpChannel"));
>      Delete();
>      return true;
>    }
> 
> +  mChannel->SetWarningReporter(this);
> +
> 
> 
> w/o it, I've run the test from bug 1328761 with the code changes from that
> bug commented out (hopefully correctly) and saw a lot of cases where
> mWarningReporter was not assigned on the channel, making
> nsHttpChannel::LogBlockedCORSRequest fail with unexpected.
> 
> There is also some inconsistency (not necessarily caused by this patch) when
> these changes are there:
> - 4 tests are failing for me
> - there is one cors failure error message when I open the web console after
> the test has finished
> - there are four (10% cases) or five (90% cases) cors error messages when I
> open the console, clear it and rerun the test (ctrl-f5 or just f5)
> 
> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp:1381
> (Diff revision 3)
> >      sPreflightCache->RemoveEntries(aURI, aRequestingPrincipal);
> >    }
> >  }
> >  
> >  nsresult
> >  nsCORSListenerProxy::StartCORSPreflight(nsIChannel* aRequestChannel,
> 
> when you are here, please add // static comment before the method definition.
> 
> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp:1486
> (Diff revision 3)
> >    rv = preHttp->
> >      SetRequestHeader(NS_LITERAL_CSTRING("Access-Control-Request-Method"),
> >                       method, false);
> >    NS_ENSURE_SUCCESS(rv, rv);
> > +  nsHttpChannel* reqCh = static_cast<nsHttpChannel*>(aRequestChannel);
> > +  nsHttpChannel* preCh = static_cast<nsHttpChannel*>(preHttp.get());
> 
> use do_QueryObject instead.
> 
> ::: netwerk/protocol/http/nsHttpChannel.h:196
> (Diff revision 3)
> >      AddSecurityMessage(const nsAString& aMessageTag,
> >                         const nsAString& aMessageCategory) override;
> > +    NS_IMETHOD LogBlockedCORSRequest(const nsAString& aMessage) override;
> >  
> > -    void SetWarningReporter(HttpChannelSecurityWarningReporter* aReporter)
> > -      { mWarningReporter = aReporter; }
> > +    void SetWarningReporter(HttpChannelSecurityWarningReporter *aReporter);
> > +      //{ mWarningReporter = aReporter; }
> 
> remove these please
> 

Still have an open issue, which is that running the testing/web-platform/tests/XMLHttpRequest/send-redirect-to-cors.htm test now results in an NS_ERROR_FILE_ACCESS_DENIED, so pushing a try run to see if anything else shows up.
(In reply to Kate McKinley [:kmckinley, :Kate] from comment #34)
> > please don't add this attr, set on the channel directly as suggested below.
> 
> This won't be in the final version, this was added to test a hypothesis.

Wasn't aware.  OK, but really remove them for the final version please.

> Still have an open issue, which is that running the
> testing/web-platform/tests/XMLHttpRequest/send-redirect-to-cors.htm test now
> results in an NS_ERROR_FILE_ACCESS_DENIED, so pushing a try run to see if
> anything else shows up.

If you want I can try to help you with this if it's reproducible locally for me.
It's 100% reproducible for me, if you can reproduce and have any suggestions, that would be a huge help, as there is nothing obvious.
(In reply to Kate McKinley [:kmckinley, :Kate] from comment #36)
> It's 100% reproducible for me, if you can reproduce and have any
> suggestions, that would be a huge help, as there is nothing obvious.

OK, I'll try to find time to reproduce and analyze soon.
Flags: needinfo?(honzab.moz)
The crash looks like it was fixed within the last 2 days.
(In reply to Honza Bambas (:mayhemer) from comment #35)
> (In reply to Kate McKinley [:kmckinley, :Kate] from comment #34)
> > > please don't add this attr, set on the channel directly as suggested below.
> > 
> > This won't be in the final version, this was added to test a hypothesis.
> 
> Wasn't aware.  OK, but really remove them for the final version please.
> 
> > Still have an open issue, which is that running the
> > testing/web-platform/tests/XMLHttpRequest/send-redirect-to-cors.htm test now
> > results in an NS_ERROR_FILE_ACCESS_DENIED, so pushing a try run to see if
> > anything else shows up.
> 
> If you want I can try to help you with this if it's reproducible locally for
> me.

This test fails the same way for me w/ and w/o your latest patch locally.  I haven't seen any NS_ERROR_FILE_ACCESS_DENIED anywhere (bash console or elsewhere)
Flags: needinfo?(honzab.moz)
Attachment #8855826 - Attachment is obsolete: true
Comment on attachment 8837981 [details]
Bug 1331730 - Log CORS messages from the content process

https://reviewboard.mozilla.org/r/112994/#review165202

no time to test this locally.  

r+ but only with the comments addressed.

::: dom/security/test/cors/browser_CORS-console-warnings.js:40
(Diff revisions 3 - 4)
>   * Set e10s related preferences in the test environment.
>   * @return {Promise} promise that resolves when preferences are set.
>   */
>  function setCookiePref() {
>    return new Promise(resolve =>
> +    // accept all cookies so that we don't fail

don't fail what exactly ? :)

::: netwerk/protocol/http/HttpChannelParent.cpp:235
(Diff revisions 3 - 4)
>      RefPtr<HttpBackgroundChannelParent> bgParent = mBgParent.forget();
>      bgParent->OnChannelClosed();
>      return;
>    }
>  
> +  // Get rid of mChannel's reference to this parent

if you want to write a comment here (the name of the method is CleanupBackgroundChannel) that don't comment on what's obvious, rather say why

::: dom/security/test/cors/browser_CORS-console-warnings.js:3
(Diff revision 5)
> +/*
> + * Description of the test:
> + *   Ensure that CORS warnings are printed to the web console where possible. This test takes the 

?

::: netwerk/base/LoadInfo.cpp
(Diff revision 5)
>  LoadInfo::SetResultPrincipalURI(nsIURI *aURI)
>  {
>    mResultPrincipalURI = aURI;
>    return NS_OK;
>  }
> -

?

::: netwerk/protocol/http/HttpChannelParent.cpp:965
(Diff revision 5)
>                                                  changedHeaders[i].mMerge);
>          }
>          MOZ_ASSERT(NS_SUCCEEDED(rv));
>        }
>  
> +      RefPtr<nsHttpChannel> nChannel = do_QueryObject(newHttpChannel);

s/nChannel/newHttpChannelObject/ ?

::: netwerk/protocol/http/HttpChannelParent.cpp:967
(Diff revision 5)
>          MOZ_ASSERT(NS_SUCCEEDED(rv));
>        }
>  
> +      RefPtr<nsHttpChannel> nChannel = do_QueryObject(newHttpChannel);
> +      if (nChannel) {
> +        nChannel->SetWarningReporter(this);

I believe this is already set in ConnectChannel.  has this particular line fixed something for you?  if yes and this is actually needed, add a comment why you do it here

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:409
(Diff revision 5)
>      mRequestingPrincipal(aRequestingPrincipal),
>      mOriginHeaderPrincipal(aRequestingPrincipal),
>      mWithCredentials(aWithCredentials && !gDisableCORSPrivateData),
>      mRequestApproved(false),
> -    mHasBeenCrossSite(false)
> +    mHasBeenCrossSite(false),
> +    mHttpChannel(nullptr)

no need to init a refptr

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:956
(Diff revision 5)
>    // Add the Origin header
>    nsAutoCString origin;
>    rv = nsContentUtils::GetASCIIOrigin(mOriginHeaderPrincipal, origin);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aChannel);
> +  nsCOMPtr <nsIHttpChannel> http = do_QueryInterface(aChannel);

excessive space after nsCOMPtr

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:1487
(Diff revision 5)
>  
>    rv = preHttp->
>      SetRequestHeader(NS_LITERAL_CSTRING("Access-Control-Request-Method"),
>                       method, false);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  RefPtr<nsHttpChannel> reqCh = do_QueryObject(aRequestChannel);

blank line before this line and a comment what and why you are doing please

::: netwerk/protocol/http/nsHttpChannel.cpp:6082
(Diff revision 5)
>      if (mCanceled) {
>          LOG(("  ignoring; already canceled\n"));
>          return NS_OK;
>      }
> +
> +    mWarningReporter = nullptr;

except a case this has to be released here from some reason (comment?) I think you have to move this release to nsHttpChannel::ReleaseListeners

::: netwerk/protocol/http/nsHttpChannel.cpp:9432
(Diff revision 5)
> +    LOG(("nsHttpChannel [this=%p] SetWarningReporter [%p]", this, aReporter));
> +    mWarningReporter = aReporter;
> +}
> +
> +HttpChannelSecurityWarningReporter*
> +nsHttpChannel::GetWarningReporter()

something tells me this should return alredy_AddRefed(), but if it's more useful and !SAFE! this way then leave it
Attachment #8837981 - Flags: review?(honzab.moz) → review+
Pushed by kmckinley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9417de5e94b4
Log CORS messages from the content process r=bz,mayhemer
https://hg.mozilla.org/mozilla-central/rev/9417de5e94b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
https://hg.mozilla.org/mozilla-central/rev/9417de5e94b4#l3.18
> +<script class="testbody" type="application/javascript;version=1.8">

Please do not add new versioned JavaScript. See bug 1342144 for details. Unfortunately, this patch actually uses versioned script features (legacy generators). So you will have to make some changes to stop using versioned scripts.
Flags: needinfo?(kmckinley)
Depends on: 1390687
Created bug 1390687 for the test changes.
Flags: needinfo?(kmckinley)
You need to log in before you can comment on or make changes to this bug.