920725 - crash in nsHtml5StreamParser::WriteStreamBytes(unsigned char const*, unsigned int, unsigned int*)

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[u279076]

This bug was filed from the Socorro interface and is 
report bp-33845ac5-085e-4aaf-ab03-c6c112130925.
=============================================================
0 	xul.dll 	nsHtml5StreamParser::WriteStreamBytes(unsigned char const *,unsigned int,unsigned int *) 	parser/html/nsHtml5StreamParser.cpp
1 	xul.dll 	nsHtml5StreamParser::WriteSniffingBufferAndCurrentSegment(unsigned char const *,unsigned int,unsigned int *) 	parser/html/nsHtml5StreamParser.cpp
2 	xul.dll 	nsHtml5StreamParser::SniffStreamBytes(unsigned char const *,unsigned int,unsigned int *) 	parser/html/nsHtml5StreamParser.cpp
3 	xul.dll 	nsHtml5StreamParser::DoDataAvailable(unsigned char const *,unsigned int) 	parser/html/nsHtml5StreamParser.cpp
4 	xul.dll 	nsHtml5StreamParser::CopySegmentsToParser(nsIInputStream *,void *,char const *,unsigned int,unsigned int,unsigned int *) 	parser/html/nsHtml5StreamParser.cpp
5 	xul.dll 	nsStringInputStream::ReadSegments(tag_nsresult (*)(nsIInputStream *,void *,char const *,unsigned int,unsigned int,unsigned int *),void *,unsigned int,unsigned int *) 	xpcom/io/nsStringStream.cpp
6 	xul.dll 	nsHtml5StreamParser::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int) 	parser/html/nsHtml5StreamParser.cpp
7 	xul.dll 	nsDocumentOpenInfo::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int) 	uriloader/base/nsURILoader.cpp
8 	xul.dll 	nsStreamListenerWrapper::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int) 	obj-firefox/dist/include/nsStreamListenerWrapper.h
9 	xul.dll 	nsHTTPCompressConv::do_OnDataAvailable(nsIRequest *,nsISupports *,unsigned __int64,char const *,unsigned int) 	netwerk/streamconv/converters/nsHTTPCompressConv.cpp
10 	xul.dll 	nsHTTPCompressConv::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int) 	netwerk/streamconv/converters/nsHTTPCompressConv.cpp
11 	xul.dll 	mozilla::net::nsHttpChannel::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,unsigned __int64,unsigned int) 	netwerk/protocol/http/nsHttpChannel.cpp
12 	xul.dll 	nsInputStreamPump::OnStateTransfer() 	netwerk/base/src/nsInputStreamPump.cpp
13 	xul.dll 	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *) 	netwerk/base/src/nsInputStreamPump.cpp
14 	xul.dll 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp
15 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
16 	xul.dll 	nsThread::ThreadFunc(void *) 	xpcom/threads/nsThread.cpp
17 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
18 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
19 	msvcr100.dll 	_callthreadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
20 	msvcr100.dll 	_threadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
21 	kernel32.dll 	BaseThreadStart 	

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsHtml5StreamParser%3A%3AWriteStreamBytes%28unsigned%20char%20const*%2C%20unsigned%20int%2C%20unsigned%20int*%29

Explosive in Firefox 25 Beta 1, currently at #4 in browser crashes, #12 overall. Other Firefox versions are reported but in much lower volume.

Unfortunately I see no correlations nor any useful comments as of yet.

Comment 1

[Tracy Walker [:tracy]]

Total Count 	URL
41 	about:blank
33 	http://static.flipora.com/websearch.html?u=&t=46.0&gl=&tv=v47&ref_type=redirect&src_type=bg&r_t=b_r
29 	http://static.flipora.com/websearch.html?u=&t=46.0&gl=in&tv=v47&ref_type=redirect&src_type=bg&r_t=b_r
27 	http://static.flipora.com/websearch.html?u=18156849&t=46.0&gl=vn&tv=v47&ref_type=redirect&src_type=bg&r_t=b_r
19 	http://static.flipora.com/websearch.html?u=8049484&t=46.0&gl=in&tv=v47&ref_type=redirect&src_type=bg&r_t=b_r
14 	http://static.flipora.com/websearch.html?u=&t=46.0&gl=id&tv=v47&ref_type=redirect&src_type=bg&r_t=b_r
12 	http://static.flipora.com/websearch.html?u=17903251&t=46.0&gl=in&tv=v47&ref_type=redirect&src_type=bg&r_t=b_r
7 	http://in.my.yahoo.com/tataindicom/dialup
7 	http://bia2ahang.org/2419/%D8%AF%D8%A7%D9%86%D9%84%D9%88%D8%AF-%D8%A2%D9%87%D9%86%DA%AF-%D8%AC%D8%AF%DB%8C%D8%AF-%D8%B3%D8%A7%D9%85%DB%8C-%D8%A8%DB%8C%DA%AF%DB%8C-%D8%A8%D8%A7-%D9%86%D8%A7%D9%85-%D9%87%D9%85%D8%A7%D9%87%D9%86.html

many more that point to static.flipora.com

Comment 2

[u279076]

I've been playing around with flipora.com but have not experienced a crash in the last hour. At this point I think it would be useful to have a developer advise us what types of functionality use this code path.

Comment 3

[Alex Keybl [:akeybl]]

Henri, you're listed as the module owner of the HTML Parser. Can you take a look at this stack and recent changes made in FF25? Thanks.

Comment 4

[Henri Sivonen (:hsivonen)]

I suspect bug 497003. sworkman, does that look plausible to you?

(How come the crash stack doesn't have line numbers?)

Comment 5

[Kyle Wooten]

Thanks!

Comment 6

[Aaron Train [:aaronmt]]

Please stop cancelling need-info and spamming bugs.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

(Sorry - just noticing this now...and probably not going to be able to do much until next Tue when I'm back in the office after summit).

From the stack trace, it looks like mLastBuffer is gone or invalid. And it's being accessed off main thread in OnDataAvailable - so, maybe something unset it ON the main thread? I see it set in OnStartRequest and then in WriteStreamBytes, which should be in the context of OnDataAvailable. OnStart and OnData should be called in sequence, never together - I don't think they're being called in parallel here. Also, I don't see mLastBuffer ever being set to nullptr (except in the constructor).

I'm wondering if the parser object is being deleted as part of a Cancel or something on the main thread?

mLastBuffer's class (nsHtml5OwningUTF16Buffer) uses nsAutoRefCnt for refcounting - since it's accessed on multiple threads, maybe it could use ThreadsafeAutoRefCnt.

All speculative musings after looking for 10 minutes :) STR would be great, of course ;) ... More next week.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Steve Workman [:sworkman] from comment #7)
> All speculative musings after looking for 10 minutes :) STR would be great,
> of course ;) ... More next week.

sadly str haven't been possible thus far according to comment 2. let us know if we can pull anything from crash stats to help in the investigation. Hopefully we can do some hardening and land before Thursday's beta, especially if the fix is only speculative.

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

So, it looks like mLastBuffer is null. From debugging one of the minidumps, I see that buffer->end is at +0x08, the crash line is calling buffer->getEnd, and all the reports crash at 0x08. So, that's nice to know.

I need to figure out why mLastBuffer could be null here and what's the simplest/safest patch we could apply.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

(Sorry, I did not intend to change those flags earlier - I was only writing a comment. And I can't set 'tracking-firefox25' back to +).

Not much more to add patch-wise, even speculative. It's unclear to me why mLastBuffer would be null and I don't have any great speculative fixes to offer. I could add a check for mLastBuffer and try to return somewhat safely - something like:

if (!mLastBuffer) {
    return NS_ERROR_UNEXPECTED;
}

so rather than crashing, the page will cause an error. But that might push the bug into the shadows.

Henri, your thoughts would be greatly appreciated here.

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Another option: the patch in Bug 915905 *may* help here. I'm wondering if we're seeing concurrent OnStartRequest and OnDataAvailable calls, which shouldn't happen. Bug 915905 aims to stop that.

Rationale: The only place I can see mLastBuffer being set to nullptr is in nsHtml5StreamParser's constructor. Then it is set to point to something in OnStartRequest, which should return before OnDataAvailable is called, before WriteStreamBytes. So, if it's null in WriteStreamBytes, maybe OnDataAvailable was called too early?

I have no other evidence to prove this, but if it's what's happening then the patch in Bug 915905 should help.

Risk: It's been on nightly for a couple of weeks with no known issues. But it hasn't baked on Aurora, so there's a risk that the larger audience on beta may find something.

I've pushed both options (1. Handle the nullptr as an error (comment 10) and 2. the patch from Bug 915905) to try.

Alex, let me know if you want to try this.

Comment 12

[u279076]

Current Ranking:
 * Release: N/A
 * Beta:    #4  (0.77%)
 * Aurora:  #38 (0.27%)
 * Nightly: N/A

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Anthony: since it's not on Nightly, then I'm a bit more convinced to try the patch from Bug 915905, pushing to Aurora and Beta.

I'll set approval-aurora/beta? in that bug.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Steve Workman [:sworkman] (PTO Fri Oct 10, 2013) from comment #7)
> mLastBuffer's class (nsHtml5OwningUTF16Buffer) uses nsAutoRefCnt for
> refcounting - since it's accessed on multiple threads, maybe it could use
> ThreadsafeAutoRefCnt.

Wouldn't that just add unnecessary locking/atomic operations, since concurrent access to instances of that class from different threads supposed to be prevented by nsHtml5StreamParser::mTokenizerMutex?

(In reply to Steve Workman [:sworkman] (PTO Fri Oct 10, 2013) from comment #11)
> Another option: the patch in Bug 915905 *may* help here. I'm wondering if
> we're seeing concurrent OnStartRequest and OnDataAvailable calls, which
> shouldn't happen. Bug 915905 aims to stop that.
> 
> Rationale: The only place I can see mLastBuffer being set to nullptr is in
> nsHtml5StreamParser's constructor. Then it is set to point to something in
> OnStartRequest, which should return before OnDataAvailable is called, before
> WriteStreamBytes. So, if it's null in WriteStreamBytes, maybe
> OnDataAvailable was called too early?
> 
> I have no other evidence to prove this, but if it's what's happening then
> the patch in Bug 915905 should help.

Seems reasonable to me that this would be the right fix.

Comment 15

[Alex Keybl [:akeybl]]

We're going to try to confirm this did the trick on Aurora (see https://bugzilla.mozilla.org/show_bug.cgi?id=915905#c9)

Comment 16

[Tracy Walker [:tracy]]

The patch in bug 915905 didn't help, per https://bugzilla.mozilla.org/show_bug.cgi?id=915905#c10

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

So, bsmedberg reports in bug 915905 comment 10 that crashes are still happening after the patch from bug 915905 landed. I see one crash on aurora with 20131013004004 and one on nightly with 20131011115413 (boo!). Both on flipora.com.

So, since that patch didn't work, what about the suggestion in comment 10:

if (!mLastBuffer) {
    return NS_ERROR_UNEXPECTED;
}

So, nsHtml5StreamParser::OnDataAvailable would return an error instead of crashing. Henri, thoughts, worries?

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Steve Workman [:sworkman] from comment #17)
> So, since that patch didn't work, what about the suggestion in comment 10:
> 
> if (!mLastBuffer) {
>     return NS_ERROR_UNEXPECTED;
> }
> 
> So, nsHtml5StreamParser::OnDataAvailable would return an error instead of
> crashing. Henri, thoughts, worries?

That's better than crashing. However, please be sure to call MarkAsBroken() before returning in order to avoid introducing a security bug by simply dropping input on the floor.

Comment 19

[Alex Keybl [:akeybl]]

Can we try to get a fix in for Thursday's beta? Last chance for speculative fixes.

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Henri, awesome. MarkAsBroken - for sure. Meant to include that.

Alex, top priority today.

Comment 21

[u279076]

Current Rank:
 * Release: N/A
 * Beta:    #4  (1.0%)
 * Aurora:  #20 (0.41%)
 * Nightly: N/A

This seems to be rising in affected branches (25, 26) since last week.

Comment 22

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Return with error in nsHtml5StreamParser::WriteStreamBytes if mLastBuffer is null

-- Return with error in nsHtml5StreamParser::WriteStreamBytes if mLastBuffer is null
-- NS_WARNING to get error output on console.
-- Calls MarkAsBroken() before returning.

try: https://tbpl.mozilla.org/?tree=Try&rev=ada03ad2a662

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08a84b468021

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 817423 [details] [diff] [review]
v1.0 Return with error in nsHtml5StreamParser::WriteStreamBytes if mLastBuffer is null

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
-- Bug 497003.
 
User impact if declined:
-- Continued crashes, seems to be most likely on flipora.com according to crash
reports.

Testing completed (on m-c, etc.):
-- Just landed on m-c; passed try push.

Risk to taking this patch (and alternatives if risky):
-- No current alternatives.
-- Crash converted to error: so, no crash, but lower visibility for us.

String or IDL/UUID changes made by this patch:
-- None.

Comment 25

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Steve Workman [:sworkman] from comment #24)
> Testing completed (on m-c, etc.):
> -- Just landed on m-c; passed try push.
> 
I meant just landed on m-i.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08a84b468021

Comment 27

[Alex Keybl [:akeybl]]

Comment on attachment 817423 [details] [diff] [review]
v1.0 Return with error in nsHtml5StreamParser::WriteStreamBytes if mLastBuffer is null

thanks Steve!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef0f77bda0be
https://hg.mozilla.org/releases/mozilla-beta/rev/23f64cd0626c

Comment 29

[u279076]

Given this was not reproducible we will evaluate whether this patch worked or not based on crashstats.

Comment 30

[Steve Workman [:sworkman] (INACTIVE)]

Fingers crossed...

Comment 31

[u279076]

This does not show up in the top-crash report for Firefox 25.0b9. It's #62 and dropping on Aurora, and non-existent on Nightly. Calling this verified fixed based on this data.

Comment 32

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Anthony!

Comment 33

[Misak Khachatryan]

This bug still crashes my Seamonkey - Linux 64 Fedora 20. Both nightly from Mozilla FTP and my own build crashing as soon as i trying to read first unread RSS feed message in mailnews.

Today's report ID is a451a278-a2db-471e-989f-b7a702140124 for example.

Comment 34

[u279076]

Steve can confirm but I'm not sure this fix was intended to eliminate this crash altogether or whether it was just meant to reduce it's prevalence. As it stands there have only been 4 reports of this signature against Seamonkey, 1 report against Thunderbird, and 2 reports against Firefox in the last 7 days.

Misak, if you can reproduce this reliably then it might be worth filing a follow-up bug but I still consider *this* report to be resolved. 

Steve, what do you think?

Comment 35

[Misak Khachatryan]

Yes, i can reproduce it without any problem 100% with official nightly and my build. Filling follow-up.

Comment 36

[Steve Workman [:sworkman] (INACTIVE)]

Yeah, Anthony's thinking was my thinking on two counts:
1. The fix was somewhat speculative, if I remember correctly. We had a good idea where the crash was happening, but not 100% accuracy on the reason.
2. A follow-up sounds like it might be helpful in determining the root cause. So, thanks for filing that.

I'll assign it to myself and continue comments over there.