537557 - [HTML5][Patch] Thread-unsafe refcounting when chardet enabled (Crash [@ nsHtml5Parser::ContinueInterruptedParsing()])

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Johnny Stenback (:jst)]

I've seen this several times already, we crash with this stack:

#3  0x0000000300000003 in ?? ()
#4  0x00007f7772219ccc in nsHtml5Parser::ContinueInterruptedParsing (this=
    0x7f774fb1f8a0) at ../../../mozilla/parser/html/nsHtml5Parser.cpp:196
#5  0x00007f777201ebd3 in nsRunnableMethod<nsContentSink, void>::Run (
    this=<value optimized out>) at ../../../dist/include/nsThreadUtils.h:282
#6  0x00007f7772703ec3 in nsThread::ProcessNextEvent (this=0x7f776cb501f0, 
    mayWait=0, result=0x7ffffb21f52c)
...

We crash with a bad mStreamParser pointer when trying to addref it, it looks like its reference count is 0 (this is an optimized build with symbols, so far I have not seen this in a debug build).

(gdb) p *mStreamParser.mRawPtr
$9 = {<nsIStreamListener> = {<nsIRequestObserver> = {<nsISupports> = {
        _vptr.nsISupports = 
    0x7f773deaee40}, <No data fields>}, <No data fields>}, <nsICharsetDetectionObserver> = {<nsISupports> = {_vptr.nsISupports = 0x0}, <No data fields>}, 
  mRefCnt = {mTagged = 0x0}, 

Seems like this got way worse when I upgraded from an old dual CPU system to a new 8 core system, so maybe threading related?

Comment 1

[Johnny Stenback (:jst)]

http://crash-stats.mozilla.com/report/index/fb0c74a1-82d5-4881-86cf-00d642091229 looks like the same problem (and is not me).

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to comment #0)
> Seems like this got way worse when I upgraded from an old dual CPU system to a
> new 8 core system, so maybe threading related?

That's odd. The parser thread isn't supposed to touch the reference count of the nsHtml5Parser instance.

Comment 3

[Johnny Stenback (:jst)]

Yeah, agreed. I've been trying to reproduce this in a debug build as well, but no luck with that yet. I've ran with XPCOM_DEBUG_BREAK=trap as well, and never hit a thread safety assertion in reference counting code etc. So I dunno why this would be related the number of available cores, or that it actually is. But I can't think of what else would be different...

This same problem (I'm guessing) also shows up with a different stack, this time the actual crash is in cycle collection code. The stack for that crash is:

#2  <signal handler called>
#3  0x00007ff6def1d44b in nsCycleCollectingAutoRefCnt::unmarkPurple (this=
    0x7ff6a609acf0) at ../../../../dist/include/nsISupportsImpl.h:214
#4  0x00007ff6df8d39d6 in AddPurpleRoot (builder=..., root=0x7ff6a609ace0)
    at ../../../mozilla/xpcom/base/nsCycleCollector.cpp:1639
#5  0x00007ff6df8d3c6d in nsPurpleBuffer::SelectPointers (this=0x7ff6d116e0b0, 
    aBuilder=...) at ../../../mozilla/xpcom/base/nsCycleCollector.cpp:911
#6  0x00007ff6df8d3d2c in nsCycleCollector::BeginCollection (this=
    0x7ff6d116e000) at ../../../mozilla/xpcom/base/nsCycleCollector.cpp:2592
#7  0x00007ff6def036ff in XPCCycleCollectGCCallback (cx=0x7ff6cb3d4400, status=
    JSGC_MARK_END)
    at ../../../../../mozilla/js/src/xpconnect/src/nsXPConnect.cpp:390
#8  0x00007ff6de655914 in js_GC (cx=0x7ff6cb3d4400, gckind=GC_NORMAL)
    at ../../../mozilla/js/src/jsgc.cpp:3090
#9  0x00007ff6def03316 in nsXPConnect::Collect (this=0x7ff6d11f27f0)
    at ../../../../../mozilla/js/src/xpconnect/src/nsXPConnect.cpp:477
#10 0x00007ff6df8d3e2c in nsCycleCollector::Collect (this=0x7ff6d116e000, 
    aTryCollections=1) at ../../../mozilla/xpcom/base/nsCycleCollector.cpp:2509
...
(gdb) f 4
#4  0x00007ff6df8d39d6 in AddPurpleRoot (builder=..., root=0x7ff6a609ace0)
    at ../../../mozilla/xpcom/base/nsCycleCollector.cpp:1639
1639        cp->UnmarkPurple(root);
(gdb) p cp
$2 = (warning: RTTI symbol not found for class 'nsHtml5StreamParser::cycleCollection'
warning: RTTI symbol not found for class 'nsHtml5StreamParser::cycleCollection'
nsXPCOMCycleCollectionParticipant *) 0x7ff6e0411c90

Could this simply be a reference counting bug somewhere? I've looked around a bit, and don't see any such problems, but I'm not sure I've seen all the uses of this yet...

Comment 4

[Henri Sivonen (:hsivonen)]

With the old parser, the nsIParser instance and the nsIStreamListener instances are the same object. With the HTML5 parser, they are distinct objects. I wonder if some code out there (e.g. in nsHTMLDocument) assumes that they can be refcounted as if they were always the same object.

Comment 5

[Henri Sivonen (:hsivonen)]

Can it be a problem than nsHtml5Parser both supports weak refs and is a cycle collection participant?

Comment 6

[Johnny Stenback (:jst)]

I doubt it, that's also the case for nsGlobalWindow, nsDocument, and probably a bunch of other classes as well.

I've been meaning to dig in a bit more here but been busy with other stuff. What I've planned to do is to add debugging code to check if this is happening on a particular url or not, but so far it hasn't been obvious how to find the url in question in the nsHtml5StreamParser code. Is there an easy way that I just haven't stumbled on yet?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #6)
> I've been meaning to dig in a bit more here but been busy with other stuff.
> What I've planned to do is to add debugging code to check if this is happening
> on a particular url or not, but so far it hasn't been obvious how to find the
> url in question in the nsHtml5StreamParser code. Is there an easy way that I
> just haven't stumbled on yet?

nsHtml5StreamParser has mDocument that could be queried for its URL from the main thread, but there's no way to get the URL from the parser thread currently.

Comment 8

[Johnny Stenback (:jst)]

Duh, ok, I somehow didn't see that when debugging one of the crashes, I'll look again. Thanks.

Comment 9

[Johnny Stenback (:jst)]

I finally got this to reproduce in a debug build. It *is* a thread safety problem. Here's the stacks I got for a couple of asserts:

#2  0x00007ffff71791b3 in NS_DebugBreak_P (aSeverity=1, aStr=
    0x7ffff75795f0 "Attempt to set a parser filter on HTML5 parser.", aExpr=
    0x7ffff75795e6 "Error", aFile=
    0x7ffff7579300 "../../../mozilla/parser/html/nsHtml5Parser.cpp", aLine=152)
    at ../../../mozilla/xpcom/base/nsDebugImpl.cpp:360
#3  0x00007ffff67fa41f in nsHtml5Parser::SetParserFilter (this=0x7fffe0f4b940, 
    aFilter=0x7fffe150e700)
    at ../../../mozilla/parser/html/nsHtml5Parser.cpp:152
#4  0x00007ffff65bcffa in nsHTMLDocument::StartAutodetection (this=
    0x7fffe2135000, aDocShell=0x7fffe0f724f8, aCharset=..., aCommand=
    0x7ffff75de3b7 "view")
    at ../../../../../mozilla/content/html/document/src/nsHTMLDocument.cpp:638
#5  0x00007ffff65be203 in nsHTMLDocument::StartDocumentLoad (this=
    0x7fffe2135000, aCommand=0x7ffff75de3b7 "view", aChannel=0x7fffe34e84c0, 
    aLoadGroup=0x7fffe0f62690, aContainer=0x7fffe0f72538, aDocListener=
    0x7fffe343f6c0, aReset=1, aSink=0x0)
    at ../../../../../mozilla/content/html/document/src/nsHTMLDocument.cpp:911
#6  0x00007ffff60c777c in nsContentDLF::CreateDocument (this=0x7fffe130c340, 
    aCommand=0x7ffff75de3b7 "view", aChannel=0x7fffe34e84c0, aLoadGroup=
    0x7fffe0f62690, aContainer=0x7fffe0f72538, aDocumentCID=..., aDocListener=
    0x7fffe343f6c0, aDocViewer=0x7fffffffce00)
    at ../../../mozilla/layout/build/nsContentDLF.cpp:440
#7  0x00007ffff60c6b02 in nsContentDLF::CreateInstance (this=0x7fffe130c340, 
    aCommand=0x7ffff75de3b7 "view", aChannel=0x7fffe34e84c0, aLoadGroup=
    0x7fffe0f62690, aContentType=0x7fffe09cd7c8 "text/html", aContainer=
    0x7fffe0f72538, aExtraInfo=0x0, aDocListener=0x7fffe343f6c0, aDocViewer=
    0x7fffffffce00) at ../../../mozilla/layout/build/nsContentDLF.cpp:234
#8  0x00007ffff6b56ac8 in nsDocShell::NewContentViewerObj (this=
    0x7fffe0f72400, aContentType=0x7fffe09cd7c8 "text/html", request=
    0x7fffe34e84c0, aLoadGroup=0x7fffe0f62690, aContentHandler=0x7fffe343f6c0, 
    aViewer=0x7fffffffce00)
    at ../../../mozilla/docshell/base/nsDocShell.cpp:7177
...


#2  0x00007ffff71791b3 in NS_DebugBreak_P (aSeverity=1, aStr=
    0x7ffff758abb0 "nsHtml5StreamParser not thread-safe", aExpr=
    0x7ffff758ab78 "_mOwningThread.GetThread() == PR_GetCurrentThread()", 
    aFile=
    0x7ffff758ab18 "../../../mozilla/parser/html/nsHtml5StreamParser.cpp", 
    aLine=76) at ../../../mozilla/xpcom/base/nsDebugImpl.cpp:360
#3  0x00007ffff6885898 in nsHtml5StreamParser::Release (this=0x7fffe0852b00)
    at ../../../mozilla/parser/html/nsHtml5StreamParser.cpp:76
#4  0x00007ffff5feb964 in nsCOMPtr<nsICharsetDetectionObserver>::~nsCOMPtr (
    this=0x7fffe36251f8, __in_chrg=<value optimized out>)
    at ../../../dist/include/nsCOMPtr.h:510
#5  0x00007ffff6d66035 in nsXPCOMDetector::~nsXPCOMDetector (this=
    0x7fffe3625190, __in_chrg=<value optimized out>)
    at ../../../../../mozilla/extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.cpp:64
#6  0x00007ffff6d65f3f in nsUniversalXPCOMDetector::~nsUniversalXPCOMDetector (
    this=0x7fffe3625190, __in_chrg=<value optimized out>)
    at ../../../../../mozilla/extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.h:146
#7  0x00007ffff6d65f6e in nsUniversalXPCOMDetector::~nsUniversalXPCOMDetector (
    this=0x7fffe3625190, __in_chrg=<value optimized out>)
    at ../../../../../mozilla/extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.h:146
#8  0x00007ffff6d66283 in nsXPCOMDetector::Release (this=0x7fffe3625190)
    at ../../../../../mozilla/extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.cpp:67
#9  0x00007ffff5fe9c98 in nsCOMPtr<nsICharsetDetector>::~nsCOMPtr (this=
    0x7fffdfd9db00, __in_chrg=<value optimized out>)
    at ../../../dist/include/nsCOMPtr.h:510
#10 0x00007ffff6887502 in nsHtml5StreamParser::FinalizeSniffing (this=
    0x7fffe0852b00, aFromSegment=0x0, aCount=0, aWriteCount=0x7fffdfd9dbdc, 
    aCountToSniffingLimit=0)
    at ../../../mozilla/parser/html/nsHtml5StreamParser.cpp:320
#11 0x00007ffff6888a7c in nsHtml5StreamParser::DoStopRequest (this=
    0x7fffe0852b00) at ../../../mozilla/parser/html/nsHtml5StreamParser.cpp:579
#12 0x00007ffff688a8b7 in nsHtml5RequestStopper::Run (this=0x7fffe1584f80)
    at ../../../mozilla/parser/html/nsHtml5StreamParser.cpp:603
#13 0x00007ffff716a369 in nsThread::ProcessNextEvent (this=0x7fffe37ecd70, 
    mayWait=1, result=0x7fffdfd9dccc)
    at ../../../mozilla/xpcom/threads/nsThread.cpp:527
#14 0x00007ffff7100f5c in NS_ProcessNextEvent_P (thread=0x7fffe37ecd70, 
    mayWait=1) at nsThreadUtils.cpp:250
#15 0x00007ffff71695da in nsThread::ThreadFunc (arg=0x7fffe37ecd70)
    at ../../../mozilla/xpcom/threads/nsThread.cpp:254
#16 0x00007ffff4b4ffaf in _pt_root (arg=0x7fffe0923de0)
    at ../../../../mozilla/nsprpub/pr/src/pthreads/./ptthread.c:228
#17 0x00000030fb006a3a in start_thread (arg=<value optimized out>)
    at pthread_create.c:297

Comment 10

[Henri Sivonen (:hsivonen)]

Thank you! It's not good at all that the chardet destructor touches the refcount of nsHtml5StreamParser from the parser thread.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Avoid refcounting chardet's observer from away the main thread

Notes:

 * This bug requires chardet to be turned on.

 * I don't know why chardet wasn't a cycle collection participant to begin with. However, since chardet is already used near CC-sensitive classes, I was shy to change its CC participation status without understanding why it isn't participating already. Therefore, I added the same kind of traversal hack the the stream parser already has for other cases where a CC participant is wrapped by a non-participant.

 * By inspection, chardet itself looks thread-safe to me. The data tables are included as code, so there aren't data files to load at run time or things like that.

 * Now chardet is instantiated (if turned on at all) for each stream parser instance even if it turns out to be useless, because it's not known if it is going to be needed at the time it needs to be instantiated to stay thread-safe.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 421232 [details] [diff] [review]
Avoid refcounting chardet's observer from away the main thread

This isn't directly related to the thread-safety problem, but please make the following change:

diff --git a/parser/html/nsHtml5StreamParser.cpp b/parser/html/nsHtml5StreamParser.cpp
--- a/parser/html/nsHtml5StreamParser.cpp
+++ b/parser/html/nsHtml5StreamParser.cpp
@@ -170,10 +170,10 @@ nsHtml5StreamParser::nsHtml5StreamParser
   if (!detectorName.IsEmpty()) {
     nsCAutoString detectorContractID;
     detectorContractID.AssignLiteral(NS_CHARSET_DETECTOR_CONTRACTID_BASE);
     AppendUTF16toUTF8(detectorName, detectorContractID);
-    mChardet = do_CreateInstance(detectorContractID.get());
-    (void) mChardet->Init(this);
+    if ((mChardet = do_CreateInstance(detectorContractID.get())))
+      (void) mChardet->Init(this);
   }
 
   // There's a zeroing operator new for everything else
 }

Without this check, if the user provides an invalid value for the intl.charset.detector setting, the browser will crash on every page (including about:config!).  Failing gracefully seems preferable.

Comment 13

[Henri Sivonen (:hsivonen)]

Thanks. Pushed with the added if:
http://hg.mozilla.org/mozilla-central/rev/d6bfc339e6e8