[:philipp] Reporter
	Description • 6 years ago

This bug was filed from the Socorro interface and is
report bp-55d515c7-c65e-49bc-bd21-c19ef0180811.
=============================================================

starting in yesterday's 63.0a1 nightly build 20180810220115 a number of content crash reports with an unsymbolized signature/stack and with Moz crash reason "Must not use a decoder that has finished." are showing up:
https://crash-stats.mozilla.com/search/?moz_crash_reason=~Must%20not%20use%20a%20decoder%20that%20has%20finished.&version=63.0a1&date=%3E%3D2018-07-01#facet-signature

	[:philipp] Reporter
	Comment 1 • 6 years ago

here's the pushlog of patches going into that nightly build:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d999fb&tochange=9a6728

	[:philipp] Reporter
	Comment 2 • 6 years ago

hi, might this be related to the work in bug 1070763?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 3 • 6 years ago

(In reply to [:philipp] from comment #2)
> hi, might this be related to the work in bug 1070763?

Most likely, yes.

twisniewski, based on the crash, it looks like we're calling into the decoder after calling it with last=true such that the last=true call has returned kInputEmpty.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 5 • 6 years ago

Try run to check that the fix to paper over multiple OnStopRequests doesn't cause other badness:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49ffe91db32f2f85f4da211dc92e23fb726ffadc

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 6 • 6 years ago

MozReview-Commit-ID: K25XhiYGzhl

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 7 • 6 years ago

It would probably be cleaner to track down the cause of multiple OnStopRequests and fix that, but let's do the easy fix to stop backing out the XHR BOM patch (which affects our wpt.fyi score on encoding tests).

	Thomas Wisniewski [:twisniewski]
	Comment 8 • 6 years ago

I agree with Henri, and think his patch will be a good stop-gap. But I may have found the actual root cause.

These are the only symbolified stack traces we have so far:
- https://crash-stats.mozilla.com/report/index/cc4bcecd-2e46-4da1-9d30-fcd340180813
- https://crash-stats.mozilla.com/report/index/f829339a-d667-4c08-ba9e-c3c6e0180813

For the second one, it's unclear what's happening, as all we know is that they are trying to read the response text in a setTimeout callback.

However, the first gives us useful clues. We hit the ChangeStateToDone here, in OnStopRequest, just after we've done the aLast=true Append:

>  if (!mResponseXML) {
>    mFlagParseBody = false;
>    if (!waitingForBlobCreation) {
>      ChangeStateToDone();
>    }

(https://hg.mozilla.org/mozilla-central/annotate/83d0673bbca49e9713f5326708cf9908f2d71764/dom/xhr/XMLHttpRequestMainThread.cpp#l2230)

The script calls GetResponseText while handling the events, which does another Append at the end of this code-fragment:

>  if ((!mResponseXML && !mErrorParsingXML) ||
>      mResponseBodyDecodedPos == mResponseBody.Length()) {
>    mResponseText.CreateSnapshot(aSnapshot);
>    return;
>  }
>  MatchCharsetAndDecoderToResponseDocument();
>  aRv = AppendToResponseText(mResponseBody.get() + mResponseBodyDecodedPos,
>                             mResponseBody.Length() - mResponseBodyDecodedPos);

(https://hg.mozilla.org/mozilla-central/annotate/83d0673bbca49e9713f5326708cf9908f2d71764/dom/xhr/XMLHttpRequestMainThread.cpp#l592)

So logically, since mResponseXML=false, that must mean mErrorParsingXML=true for the first clause to fail. And the only place mErrorParsingXML is set to true is at the end of OnStopRequest, after this code would run. The only way I can see this happening is if the user is re-using the same XHR object, which fails parsing once (empty/bad XML file), then doesn't fail, and hits this case. And indeed, mErrorParsingXML is never reset to false.

I'm trying to engineer a crashtest for this now.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 6 years ago

Comment on attachment 8999640 [details]
Bug 1482640 - Null out mDecoder in OnStopRequest to avoid asserting when there is more than one OnStopRequest.

Olli Pettay [:smaug] has approved the revision.

	Thomas Wisniewski [:twisniewski]
	Comment 10 • 6 years ago

So in order for the if-statement to fail, this clause must also be false:

>mResponseBodyDecodedPos == mResponseBody.Length()

But those variables are reset to 0 on each send() in ResetResponse(), and I see no way for them to get out of sync unless the response is XML (which begs the question of how we even got past the !mResponseXML to get to this code in the first place).

That is to say that I'm completely mystified how the stack trace gets here. But given that the other stack trace we have involves timeouts, I think it's safe to say that it's happening in the wild regardless. Henri, I think we should also make sure to clear mErrorParsingXML in at least the ResetResponse method (if not everywhere where we set mResponseXML=nullptr).

I'll continue trying to figure out a crashtest for now, as it would be great to have test coverage for this.

	Pulsebot
	Comment 11 • 6 years ago

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02b3167c1a8e
Null out mDecoder in OnStopRequest to avoid asserting when there is more than one OnStopRequest. r=smaug

	Thomas Wisniewski [:twisniewski]
	Comment 12 • 6 years ago

I'm afraid I can't reproduce a crashtest here, so I think we'd need a URL with a live test-case to try to reproduce it. I still feel we should clear out the mErrorParsingXML variable as I mentioned in comment 8, however. Henri, what do you think?

	Eliza Balazs [:ebalazs_]
	Comment 13 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/02b3167c1a8e

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 14 • 6 years ago

(In reply to Thomas Wisniewski [:twisniewski] (PTO Aug 15- Sep 7) from comment #12)
> I'm afraid I can't reproduce a crashtest here, so I think we'd need a URL
> with a live test-case to try to reproduce it. I still feel we should clear
> out the mErrorParsingXML variable as I mentioned in comment 8, however.
> Henri, what do you think?

Looks like doing something in that area is needed. Let's do that in bug 1483764.