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

Status: RESOLVED FIXED

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

Description

[Kevin Brosnan]

Loading http://www.gokbayrock.com 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

Comment 1

[Kevin Brosnan]

The page just prints 

   Warning: fgets(): supplied argument is not a valid stream resource in 
/var/www/gokbayrock.com/wp-content/plugins/visitor-likedislike-post-rating/visitor-likedislike-post-rating.php on line 108

without end.

Comment 2

[Henri Sivonen (:hsivonen)]

http://crash-stats.mozilla.com/report/index/d77accad-570c-42fb-9251-b460c2100711 has a non-sensical stack.

http://crash-stats.mozilla.com/report/index/9f65969f-5bc9-4b7c-b26e-8a23a2100711 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.

Comment 3

[timeless]

This should be caused by "next" link confusion, perhaps in:
232 nsHtml5Parser::Parse(const nsAString& aSourceBuffer,

307   nsHtml5UTF16Buffer* prevSearchBuf = nsnull;
308   nsHtml5UTF16Buffer* searchBuf = mFirstBuffer;
309 
310   // after document.open, 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).

Comment 4

[Henri Sivonen (:hsivonen)]

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.

Comment 5

[timeless]

the goal of the pattern is to enable this:

nsFoo::~nsFoo() {
  mBar->RemoveParent(this);
}

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

looking at it,
nsIFoo *sFoo;

[ sFoo->mRefCnt = 1 ]

NS_RELEASE(sFoo)

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).

Comment 6

[Henri Sivonen (:hsivonen)]

Spun off the other crash to bug 579876. Focused this one to be only about the crash already discussed.

Comment 7

[Johnny Stenback (:jst)]

Doesn't seem to be happening here. What's the trigger? Loading the given url several times doesn't seem to be enough...

Comment 8

[Kevin Brosnan]

This page was looping a php error infinitely resulting in very large files +20mb in a manner of minutes.

Comment 9

[[e-mail]]

-> me for investigation.

Comment 10

[[e-mail]]

Can't repro since the URL is just an empty page.

Comment 11

[Kevin Brosnan]

Attachment: Sample php file that is similar to what the server was outputing

Have it running on http://kbrosnan.mine.nu:8080/tmp/577952/ 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.

Comment 12

[[e-mail]]

Is this Windows-only? (Rather, has anyone tried on other plats?) Opt/debug?

Comment 13

[[e-mail]]

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?)

Comment 14

[[e-mail]]

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.

Comment 15

[[e-mail]]

Attachment: patch

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?

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 478429 [details] [diff] [review]
patch

Seems appropriate to me. Thanks for looking into this.

Comment 17

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/a8c08c2f18b7

Going to leave this open for a bit to see if the crashes disappear.

Comment 18

[[e-mail]]

Doesn't look like this is fixed:

https://crash-stats.mozilla.com/report/index/e6d38479-3782-4145-81dd-3c5332101011

Comment in that report is a link to https://bugzilla.mozilla.org/show_bug.cgi?id=597849#c5... 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&) ] (https://crash-stats.mozilla.com/report/index/43c808f3-fb28-43dc-ad17-cac172101011).

Comment 19

[[e-mail]]

I did manage to get a crash ~nsHtml5UTF16Buffer using those STR: http://crash-stats.mozilla.com/report/index/bp-3122dd54-9337-498e-99af-efee82101011

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.

Comment 20

[[e-mail]]

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...

Comment 22

[[e-mail]]

I think this was fixed by my patch. I'm not seeing any deep-recursion stacks from recent builds.