Closed Bug 577952 Opened 10 years ago Closed 9 years ago

Deep recursion in Release crashes on double-free in [@ nsHtml5UTF16Buffer::~nsHtml5UTF16Buffer()]


(Core :: DOM: HTML Parser, defect, P3, critical)

Windows 7



Tracking Status
blocking2.0 --- betaN+


(Reporter: kbrosnan, Assigned: dwitte)




(Keywords: crash)

Crash Data


(2 files)

Loading causes the trunk to crash after a few seconds.

bp-d77accad-570c-42fb-9251-b460c21007117/11/201012:51 PM
bp-9f65969f-5bc9-4b7c-b26e-8a23a21007117/11/201012:50 PM
The page just prints 

   Warning: fgets(): supplied argument is not a valid stream resource in 
/var/www/ on line 108

without end. has a non-sensical stack. looks like a potential bad index to the tree builder stack. Too bad the server is no longer responding, so I can't reproduce the crash.
This should be caused by "next" link confusion, perhaps in:
232 nsHtml5Parser::Parse(const nsAString& aSourceBuffer,

307   nsHtml5UTF16Buffer* prevSearchBuf = nsnull;
308   nsHtml5UTF16Buffer* searchBuf = mFirstBuffer;
310   // after, the first level of document.write has null key
311   if (aKey) {
312     while (searchBuf != mLastBuffer) {
313       if (searchBuf->key == aKey) {
314         // found a key holder
315         // now insert the new buffer between the previous buffer
316         // and the key holder.
317         buffer->next = searchBuf;
318         if (prevSearchBuf) {
319           prevSearchBuf->next = buffer;
320         } else {
321           mFirstBuffer = buffer;
322         }

the problem is that:
170839 	xul.dll 	nsHtml5UTF16Buffer::Release 	parser/html/nsHtml5UTF16BufferCppSupplement.h:86
170840 	xul.dll 	nsHtml5UTF16Buffer::`scalar deleting destructor' 	
170841 	xul.dll 	nsHtml5UTF16Buffer::Release 	parser/html/nsHtml5UTF16BufferCppSupplement.h:86
170842 	xul.dll 	nsHtml5StreamParser::ContinueAfterScripts 	parser/html/nsHtml5StreamParser.cpp:1004
170843 	xul.dll 	nsHtml5Parser::ParseUntilBlocked 	parser/html/nsHtml5Parser.cpp:604
170844 	xul.dll 	nsHtml5TreeOpExecutor::RunFlushLoop 	parser/html/nsHtml5TreeOpExecutor.cpp:457

we're releasing the last "reference" to our object
79 nsHtml5UTF16Buffer::Release()
80 {
81   NS_PRECONDITION(0 != mRefCnt, "Release without AddRef.");
82   --mRefCnt;
83   NS_LOG_RELEASE(this, mRefCnt, "nsHtml5UTF16Buffer");

so we enter here:
84   if (mRefCnt == 0) {

this sets the ref count back to 1, which means we can reenter this block later:
85     mRefCnt = 1; /* stabilize */

now we call delete:
86     delete this;

which deletes the object which involves dealing w/ member variables:
61 class nsHtml5UTF16Buffer
62 {
64     PRUnichar* buffer;
65     PRInt32 start;
66     PRInt32 end;
79 #include "nsHtml5UTF16BufferHSupplement.h"

here we have next:
42     nsRefPtr<nsHtml5UTF16Buffer> next;

which has refcount 1 from "stabilize", so we perform nsRefPtr::~nsRefPtr which calls nsHtml5UTF16Buffer::Release, and we go to line 82/84/85/86/42

43     void* key;

47     nsAutoRefCnt mRefCnt;

next happens to be the same as this (or it's intimately connected to this, whichever).
I fail to locate anything that'd introduce a reference loop. I fail to see how the double-release condition could arise. However, I also fail to see why the refcount code pattern is to stabilize at 1 instead of e.g. MAX_INT.
the goal of the pattern is to enable this:

nsFoo::~nsFoo() {

nsBar::RemoveParent(nsIFoo *aFoo) {
  nsCOMPtr<nsIFoo> strongFoo(aFoo);

looking at it,
nsIFoo *sFoo;

[ sFoo->mRefCnt = 1 ]


in nsFoo::Release,
sFoo->mRefCnt = 0

and we call nsFoo::~nsFoo (via delete)

in nsFoo::~nsFoo
call nsBar::RemoveParent

in nsBar::RemoveParent
call nsFoo::AddRef via nsCOMPtr::nsCOMPtr

[ sFoo->mRefCnt = 2 ]

call nsFoo::Release via nsCOMPtr::~nsCOMPtr

[ sFoo->mRefCnt = 1 ]

If at this point, mRefCnt reaches 0, we'd call delete again and reenter nsFoo::~nsFoo, which is bad (it'd look pretty much like what we have here actually).
Spun off the other crash to bug 579876. Focused this one to be only about the crash already discussed.
Priority: -- → P3
Summary: Page crashes on load [@ nsHtml5TreeBuilder::appendToCurrentNodeAndPushFormattingElementMayFoster(int, nsHtml5ElementName*, nsHtml5HtmlAttributes*) ] → Deep recursion in Release crashes on double-free in [@ nsHtml5UTF16Buffer::~nsHtml5UTF16Buffer()]
Doesn't seem to be happening here. What's the trigger? Loading the given url several times doesn't seem to be enough...
blocking2.0: ? → betaN+
This page was looping a php error infinitely resulting in very large files +20mb in a manner of minutes.
-> me for investigation.
Assignee: nobody → dwitte
Can't repro since the URL is just an empty page.
Have it running on though all I can do is OOM crash Firefox with it. Warning it will send you 100s of MB of html in a very short amount of time.
Is this Windows-only? (Rather, has anyone tried on other plats?) Opt/debug?
I also don't see how a reference cycle would cause a crash here. It'd just leak.

Can't repro on a Linux64 debug or opt build. (The page isn't feeding me data at a very fast rate, though... probably because it's far away?)
No proof of this yet, but what could be happening is:

1) nsHtml5Parser::mLastBuffer (weak pointer) gets assigned to the (strongly owned) 'next' field of another nsHtml5UTF16Buffer.
2) This new nsHtml5UTF16Buffer eventually gets freed.
3) Its 'next' field is the last reference to mLastBuffer, so it gets freed.
4) nsHtml5Parser performs some operation that assigns the stale value of mLastBuffer into a strongly-held reference.
5) The owning object goes away, and it gets double-freed.

Not sure of the ownership model of nsHtml5UTF16Buffer -- making mLastBuffer a strong reference might fix this, but it might also cause a reference cycle that holds buffers alive as long as the nsHtml5Parser object survives. (May not be an issue.) But it certainly won't fix the use of a stale value, if that exists.
Attached patch patchSplinter Review
I'm not sure if this will fix it, but I don't think it can hurt. I can't reproduce, so it's hard to tell.

Henri, what do you think?
Attachment #478429 - Flags: review?(hsivonen)
Comment on attachment 478429 [details] [diff] [review]

Seems appropriate to me. Thanks for looking into this.
Attachment #478429 - Flags: review?(hsivonen) → review+

Going to leave this open for a bit to see if the crashes disappear.
Doesn't look like this is fixed:

Comment in that report is a link to unclear if that means that's what the user was doing at the time of crash. Following the STR in that comment did crash me, but the stack was [@ nsPurpleBuffer::SelectPointers(GCGraphBuilder&) ] (
I did manage to get a crash ~nsHtml5UTF16Buffer using those STR:

It's not a deep recursion stack, though. It's parented by 	nsHtml5Parser::ParseUntilBlocked, like most of the other stacks I get from those STR.
After looking at the other stacks I crashed in, I'm thinking this is a memory-stomping problem elsewhere. I'm getting all kinds of seemingly unrelated stacks, mostly in free(). We probably want R&R here to figure out what touches the pointers that we later try to free...
I think this was fixed by my patch. I'm not seeing any deep-recursion stacks from recent builds.
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsHtml5UTF16Buffer::~nsHtml5UTF16Buffer()]
You need to log in before you can comment on or make changes to this bug.