1153484 - Assertion failure: !mMessage, at ErrorResult.h:54, due to bad error handling in mozilla::dom::(anonymous namespace)::FillResponseHeaders::VisitHeader (FetchDriver.cpp)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Steps to reproduce:
 1. in a debug build, go to https://github.com/new
 2. create a repository
 3. wait for the resulting page to load

Actual results: crash due to fatal assertion about half the time:
Assertion failure: !mMessage, at ../../../dist/include/mozilla/ErrorResult.h:54


This is due to failing to properly deal with the ErrorResult in:

mozilla::dom::(anonymous namespace)::FillResponseHeaders::VisitHeader (in FetchDriver.cpp), which is currently:

627	  NS_IMETHOD
628	  VisitHeader(const nsACString & aHeader, const nsACString & aValue) override
629	  {
630	    ErrorResult result;
631	    mResponse->Headers()->Append(aHeader, aValue, result);
632	    return result.ErrorCode();
633	  }

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: gdb session

Comment 2

[Boris Zbarsky [:bzbarsky]]

Uh, yeah.  That "return foo.ErrorCode()" is only OK if you're exactly duplicating behavior that could be accomplished with xpconnect.  If you're throwing exceptions that have more than an nsresult to them, you're not allowed to do that.

In an opt build, this is leaking, of course, instead of asserting.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just hit this again, filing a new issue on github at https://github.com/TheoChevalier/FoxShop/issues/new , typing some text in the issue title and body, and then switching to the "Preview" tab.

It's pretty annoying to be unable to use github in a debug build without fear of crashing...

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I think we can clear the message here, but unfortunately it seems the nsIHttpHeaderVisitor does not have a way to propagate the message itself.

Comment 5

[Boris Zbarsky [:bzbarsky]]

You just shouldn't have XPCOM methods calling webidl ones that can do interesting things with the exception.  Probably best to refactor the code to have an XPCOM version of Append and a WebIDL one or something so the issue doesn't arise.

But yes, a stopgap is to ClearMessage and return an nsresult that might mean something to the XPCOM consumer.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Actually, I think there is a bigger problem here.  The nsIHttpHeaderVisitor.idl indicates the returned error is just used to stop enumeration.  This method probably needs to ignore invalid headers by eating the error or flag the error somewhere else.  Right now its ignore the invalid header and all following headers which is probably wrong.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fetch should ignore invalid headers, but still process later headers. r=nsm

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Are you guaranteed that result is an error-with-message here?

I think what we should do is add a Catch() or ClearError() or SuppresException() or something method on ErrorResult which will properly handle dealing with the exception no matter what sort of exception we have (e.g. ClearMessage in the IsErrorWithMessage() case, unroot in the IsJSException() case, etc).  That will make it much clearer what's actually going on.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fetch should ignore invalid headers, but still process later headers. r=nsm

Updated patch that clarifies we should expect channel->VisitResponseHeaders() to never return an error.

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596609 [details] [diff] [review]
Fetch should ignore invalid headers, but still process later headers. r=nsm

We're addressing comment 8 in a follow-up.  See bug 1157754.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596609 [details] [diff] [review]
Fetch should ignore invalid headers, but still process later headers. r=nsm

Try shows some unexpected errors.  Seems we might be trying to get the headers from the channel before its ready.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fetch should ignore invalid headers, but still process later headers. r=nsm

Go back to the origin patch.  VisitResponseHeaders can still fail if the channel is canceled or failed to get an mResponseHead for other reasons.

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a43a1dafb8

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7a43a1dafb8

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8596701 [details] [diff] [review]
Fetch should ignore invalid headers, but still process later headers. r=nsm

Approval Request Comment
[Feature/regressing bug #]: Fetch API
[User impact if declined]: Possible unexpected results on sites using fetch() such as github due to losing expected HTTP headers.
[Describe test coverage new/current, TreeHerder]: Existing fetch() tests, although I don't think we explicitly test this particular branch of the code.
[Risks and why]: Minimal.  This is fixing error handling that was already in place.
[String/UUID change made/needed]: None.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8596701 [details] [diff] [review]
Fetch should ignore invalid headers, but still process later headers. r=nsm

Approved for uplift to 39; sounds like a low-risk fix to error handling.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9242257d7697