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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dbaron, Assigned: bkelly)

Tracking

({assertion, crash, memory-leak})

Trunk
mozilla40
x86_64
Linux
assertion, crash, memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 unaffected, firefox39 fixed, firefox40 fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

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	  }
(Assignee)

Updated

3 years ago
Blocks: 1059784
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.
Keywords: mlk
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...
(Assignee)

Comment 4

3 years ago
I think we can clear the message here, but unfortunately it seems the nsIHttpHeaderVisitor does not have a way to propagate the message itself.
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.
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8596580 [details] [diff] [review]
Fetch should ignore invalid headers, but still process later headers. r=nsm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42f82fbb7083
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8596580 - Flags: review?(nsm.nikhil)
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.
(Assignee)

Comment 9

3 years ago
Created attachment 8596609 [details] [diff] [review]
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
Attachment #8596580 - Attachment is obsolete: true
Attachment #8596580 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 10

3 years ago
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.
Attachment #8596609 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 11

3 years ago
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.
Attachment #8596609 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 12

3 years ago
Created attachment 8596701 [details] [diff] [review]
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
Attachment #8596609 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8596701 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
status-firefox38: --- → unaffected
status-firefox39: --- → affected
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7a43a1dafb8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 15

3 years ago
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.
Attachment #8596701 - Flags: approval-mozilla-aurora?
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.
Attachment #8596701 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.