669903 - double unlink crashes [@ nsGenericHTMLElement::ClearDataset()] nsDOMStringMap's Unlink

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

This bug was filed from the Socorro interface and is 
report bp-66910cea-e075-42a7-919b-98a242110705 .
============================================================= 
0 	xul.dll 	nsGenericHTMLElement::ClearDataset 	content/html/content/src/nsGenericHTMLElement.cpp:394
1 	xul.dll 	nsDOMStringMap::cycleCollection::Unlink 	content/html/content/src/nsDOMStringMap.cpp:54
2 	xul.dll 	nsCycleCollector::CollectWhite 	xpcom/base/nsCycleCollector.cpp:1962
3 	xul.dll 	nsCycleCollector::FinishCollection 	xpcom/base/nsCycleCollector.cpp:2784
4 	xul.dll 	nsCycleCollectorRunner::Collect 	xpcom/base/nsCycleCollector.cpp:3437
5 	xul.dll 	nsCycleCollector_collect 	xpcom/base/nsCycleCollector.cpp:3503
6 	xul.dll 	nsJSContext::CycleCollectNow 	dom/base/nsJSEnvironment.cpp:3271
7 	xul.dll 	CCTimerFired 	dom/base/nsJSEnvironment.cpp:3311
8 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:424
9 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:520
10 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:617
11 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:134
12 	xul.dll 	xul.dll@0xb79167 	
13 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
14 	xul.dll 	xul.dll@0x37125f 	
15 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:176
16 	xul.dll 	nsEvent::nsEvent 	obj-firefox/dist/include/nsGUIEvent.h:569
17 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
18 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:256
19 		@0x7530ffff 

While testing, I'm seeing this crash occuring. Sorry, atm I don't have a testcase for this crash.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Is this a null pointer crash? null slots (maybe even null element) + offset to
mDataset.

Is it possible that something is keeping nsDOMStringMap alive after
first unlink. Then it gets unlinked again, and mElement is already null.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, looking at the code I couldn't make sense of this.

As long as the dataset is alive, the element should be alive too. *Especially* in the places where ClearDataset is called since we only ever clear the strong mElement reference after calling ClearDataset.

And as long as the dataset is alive, the element should be pointing to it, as that pointer is set up as soon as the dataset is created, and only cleared once the dataset goes away.

And the slots are only deleted when the element is deleted, which again should only happen when the dataset is going away.


Or, at least that was my initial analysis. I just realized that there is one situation when this isn't true. Specifically, if we end up cycle collecting a dataset more than once we'll end up dereferencing a null mElement pointer in the unlink code. I suppose that can happen.

So what we should do is to add a if (mElement) check to the unlink code and see if that helps.


William, can you try that?

Comment 3

[Martijn Wargers (dead)]

Sorry, I haven't been able to get a minimized or unminimized testcase yet.
I can tell you that in order for this crash to trigger is when I do something like this setTimeout(function(){document.write(document.documentElement.innerHTML)}, 1000);. And the testcase probably keeps a whole lot of dataset references somewhere.

Comment 4

[William Chen [:wchen]]

Attachment: Possible fix for crash

I added a check for the element prior to clearing the dataset. No test case because I was not able to reproduce the crash. I tried a few things but I was not able to get the unlink code to run more than once.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 548231 [details] [diff] [review]
Possible fix for crash

Add an assertion as well, as there's definitely a bug somewhere if we unlink twice.

Comment 6

[William Chen [:wchen]]

Attachment: Possible fix for crash

Added assertion and comment.

Comment 7

[William Chen [:wchen]]

Attachment: Possible fix for crash

Forgot to add r=sicking

Comment 8

[Peter Van der Beken [:peterv]]

Is this ready to land?

Comment 9

[Andrew McCreight [:mccr8]]

Jesse came up with a test case that looks very similar to this in bug 706281, that only triggers a crash after I landed the weak map cycle collector integration in bug 668855.

Comment 11

[Andrew McCreight [:mccr8]]

In the first CC in Jesse's test case, the CC identifies the nsDOMStringMap as garbage, so it is unlinked.  There's only one reference to the DOMStringMap, an XPCWrappedNative, which is also identified as garbage and has a single reference.  But both of them show up again in the next CC.  I'm not sure why that is.  So you end up with a double unlink.  The attached patch seems like the way to go.  Double unlinks are bad, but they do happen.  I know of a few ways they can be caused.

Comment 12

[Jesse Ruderman]

How bad are double unlinks? Should the assertion be removed from the patch, or should a bug be filed on fixing the assertion?

Comment 13

[Andrew McCreight [:mccr8]]

They are kind of bad, but probably not anything worse than a controlled crash.  Unlinks usually just null out references, so I think the worst thing that will happen is a null deref.  But I don't think anybody has really audited unlinks.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I've always tried to write unlink code so that it can handle more than one unlinking -
I think most of our unlink handling is like that.

Comment 15

[Andrew McCreight [:mccr8]]

Maybe I'm reading the code wrong, but it looks like the wrappee of an XPCWrappedNative isn't unlinked by the XPCWN's unlink, so it will stay alive until the next GC.  Which means that double unlinks are probably fairly common.  I don't know.  One XPCWN I looked at disappeared from one log to another, but a second one didn't.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: check for double unlink

I just removed the assertion from William's last patch.  Double unlinks will happen whenever a wrapped native is marked as garbage, and the cycle collector runs twice in a row without a GC in between, so I think it shouldn't be an assertion.

Comment 17

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/271ba479719f

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 579901 [details] [diff] [review]
check for double unlink

Low risk fix (add a null check), but also an extremely rare crash, so I am flagging for Aurora but not Beta.  I could only find 9 crashes with this signature in the last week across all versions.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/271ba479719f

Comment 20

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/28320cfb9cac

Comment 21

[u279076]

Is this something QA can verify?

Comment 22

[Andrew McCreight [:mccr8]]

The steps in bug 706281 comment 0 triggered this crash.

Comment 23

[Ioana (away)]

Verified as fixed with the steps referenced in comment 22 on:
Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux i686; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0) Gecko/20100101 Firefox/10.0

I have also verified the crash stats from the Socorro interface and this crash didn't reproduce on Firefox 10 anymore.

Comment 24

[Ioana (away)]

Verified as fixed with the steps referenced in comment 22 on:
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

Comment 25

[Ioana (away)]

The crash status from the Socorro interface were also verified and this crash didn't happen on Firefox 10 or newer anymore.