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

RESOLVED FIXED

Status

()

P3
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: kbrosnan, Assigned: dwitte)

Tracking

({crash})

Trunk
x86
Windows 7
crash
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
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.
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

8 years ago
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).
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

8 years ago
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).
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+
(Reporter)

Comment 8

8 years ago
This page was looping a php error infinitely resulting in very large files +20mb in a manner of minutes.
(Assignee)

Comment 9

8 years ago
-> me for investigation.
Assignee: nobody → dwitte
(Assignee)

Comment 10

8 years ago
Can't repro since the URL is just an empty page.
(Reporter)

Comment 11

8 years ago
Created attachment 469735 [details]
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.
(Assignee)

Comment 12

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

Comment 13

8 years ago
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?)
(Assignee)

Comment 14

8 years ago
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.
(Assignee)

Comment 15

8 years ago
Created attachment 478429 [details] [diff] [review]
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?
Attachment #478429 - Flags: review?(hsivonen)
Comment on attachment 478429 [details] [diff] [review]
patch

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

Comment 17

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a8c08c2f18b7

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

Comment 18

8 years ago
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).
(Assignee)

Comment 19

8 years ago
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.
(Assignee)

Comment 20

8 years ago
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...
(Assignee)

Comment 22

8 years ago
I think this was fixed by my patch. I'm not seeing any deep-recursion stacks from recent builds.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsHtml5UTF16Buffer::~nsHtml5UTF16Buffer()]
You need to log in before you can comment on or make changes to this bug.