945585 - Crash involving nsNodeUtils::LastRelease(), possibly due to running it twice

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

A user reported frequent crashes.  He provided a test case which he requested not be made public, but I can email anyone the details.  I've given them to bz.

Stack at crash time:

#0  0x000000000040fdee in jemalloc_crash ()
    at ../../../memory/mozjemalloc/jemalloc.c:1592
#1  0x000000000040e916 in arena_dalloc_small (chunk=<optimised out>, 
    arena=<optimised out>, ptr=<optimised out>, mapelm=<optimised out>)
    at ../../../memory/mozjemalloc/jemalloc.c:4610
#2  arena_dalloc (ptr=<optimised out>, offset=<optimised out>)
    at ../../../memory/mozjemalloc/jemalloc.c:4678
#3  0x00007ffff2f99b19 in nsNodeUtils::LastRelease (aNode=0x7fffbe407000)
    at ../../../../content/base/src/nsNodeUtils.cpp:193
#4  0x00007ffff2f55cc4 in nsDocument::Release (this=0x7fffbe407000)
    at ../../../../content/base/src/nsDocument.cpp:1621
#5  0x00007ffff317a024 in ~nsCOMPtr_base (this=<optimised out>)
    at ../../../../dist/include/nsCOMPtr.h:430
#6  ~nsCOMPtr (this=<optimised out>)
    at ../../../../dist/include/nsCOMPtr.h:469
#7  ~nsRefPtr (this=<optimised out>, this=<optimised out>, 
    this=<optimised out>, this=<optimised out>, this=<optimised out>)
    at ../../../../dist/include/nsCOMPtr.h:469
#8  TX_CompileStylesheet (aNode=<optimised out>, aProcessor=<optimised out>, 
    aCallerPrincipal=0x0, aStylesheet=0x7fffbe6944f0)
    at ../../../../../content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp:744
#9  0x00007ffff317f2bf in txMozillaXSLTProcessor::NodeWillBeDestroyed (
    this=0x7fffbe6944c0, aNode=0x100000)
    at ../../../../../content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:1199
#10 0x00007ffff2f99aee in nsNodeUtils::LastRelease (aNode=0x7fffbe407000)
    at ../../../../content/base/src/nsNodeUtils.cpp:188
#11 0x00007ffff2f55cc4 in nsDocument::Release (this=0x7fffbe407000)
    at ../../../../content/base/src/nsDocument.cpp:1621
#12 0x00007ffff3b82775 in ReleaseSliceNow (aSlice=<optimised out>, 
    aData=0x7fffbe2a4798)
    at ../../../xpcom/base/CycleCollectedJSRuntime.cpp:1064
#13 0x00007ffff3b82a06 in mozilla::IncrementalFinalizeRunnable::ReleaseNow (
    this=0x7fffbe2a4780, aLimited=true)
    at ../../../xpcom/base/CycleCollectedJSRuntime.cpp:1126
#14 0x00007ffff3b82b1f in mozilla::IncrementalFinalizeRunnable::Run (
    this=0x7fffbe2a4780)
    at ../../../xpcom/base/CycleCollectedJSRuntime.cpp:1159
#15 0x00007ffff3b7e8ec in nsThread::ProcessNextEvent (this=0x7ffff6c27f20, 
    mayWait=<optimised out>, result=0x7fffffff6dc0)
    at ../../../xpcom/threads/nsThread.cpp:622
#16 0x00007ffff3b4b0e8 in NS_ProcessNextEvent (thread=<optimised out>, 
    mayWait=true) at ../../../xpcom/glue/nsThreadUtils.cpp:238
#17 0x00007ffff2fc142b in nsXMLHttpRequest::Send (this=0x7fffc405ac00, 
    aVariant=<optimised out>, aBody=...)
    at ../../../../content/base/src/nsXMLHttpRequest.cpp:2915
#18 0x00007ffff3a855f7 in Nullable (aBody=..., this=<optimised out>, 
    this=<optimised out>, this=<optimised out>, aValue=...)
    at ../../../content/base/src/nsXMLHttpRequest.h:350
#19 Send (this=<optimised out>, rv=<optimised out>, this=<optimised out>, 
    aBody=...) at ../../../content/base/src/nsXMLHttpRequest.h:354


jemalloc is asserting;  it looks like a double-free or use-after-free.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Valgrind output

Comment 2

[Boris Zbarsky [:bzbarsky]]

In particular, double-LastRelease on the document?

Comment 3

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

Attachment: doc_release.diff

Would like to see the testcase ofc, but this makes document's Release work the
way it works with other nodes.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8341623 [details] [diff] [review]
doc_release.diff

Review of attachment 8341623 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Nick and Olli for looking into this!

I sent Olli the test case via mail.

::: content/base/src/nsDocument.cpp
@@ +1647,5 @@
>        mNeedsReleaseAfterStackRefCntRelease = true;
>        NS_ADDREF_THIS();
>        return mRefCnt.get();
>      }
>      NS_ASSERT_OWNINGTHREAD(nsDocument);

While you are here, you could delete this second assert that looks redundant.

Comment 5

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

Attachment: removed assert

Still testing...

Comment 6

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

So far got 3 fatal assertions on debug build without the patch and none with the patch.
Now testing opt builds.

Comment 7

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

opt build without patch crashes, with doesn't.

Comment 8

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

Comment on attachment 8341686 [details] [diff] [review]
removed assert

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'd say not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment could be to stabilize refcnt during shutdown

Which older supported branches are affected by this flaw?
25+

If not all supported branches, which bug introduced the flaw?
Bug 789919

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch applies on beta/aurora/nightly

How likely is this patch to cause regressions; how much testing does it need?
Should be quite safe

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 789919
User impact if declined: 
crashes
Testing completed (on m-c, etc.): 
NA
Risk to taking this patch (and alternatives if risky): 
Should be quite safe. Adds stabilizing to document's Release. Similar to what it had >=24
String or IDL/UUID changes made by this patch:
NA

Comment 9

[(no longer active)]

Is there a way for us to detect this kind of bug in the future?  Perhaps by asserting the non-zero-ness of the refcount in nsNodeUtils::LastRelease?

Comment 10

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

That we could do. nsDocument is very special case here though, see Bug 830975. Other nodes use normal macros.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

We're at the end of a cycle and shipping in a week. This needs to go in a couple of weeks into the new cycle.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8341686 [details] [diff] [review]
removed assert

sec-approval+ for checkin on 12/23 or later into trunk.

Comment 13

[Nicholas Nethercote [inactive]]

> sec-approval+ for checkin on 12/23 or later into trunk.

Are you saying this can't be checked into m-c until after 12/23?  That can't be right... surely this should land on m-c and aurora immediately, at least?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The goal is to minimize the time the patch is world visible before we ship the fix.  So if we're not going to ship the fix in the release going out next week, yes, this needs to wait a while.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

(In reply to Nicholas Nethercote [:njn] from comment #13)
 
> Are you saying this can't be checked into m-c until after 12/23?  That can't
> be right... surely this should land on m-c and aurora immediately, at least?

This is exactly what I'm saying. The point of sec-approval? is to gate when we check in bad security issues during our six week cycles. Since we're shipping in a week, it is too late to go in now. The date was chosen to give time for it to get in without too much exposure. We do this all the time.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Oh, and when it goes into Trunk should be just before it goes into branches since exposure in one place is exposure everywhere to anyone monitoring checkins of hidden bugs.

Comment 17

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c7839e7f32e

Comment 18

[Andrew McCreight [:mccr8]]

followup for bustage

https://hg.mozilla.org/integration/mozilla-inbound/rev/697f5c9e6ec1

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c7839e7f32e
https://hg.mozilla.org/mozilla-central/rev/697f5c9e6ec1

Comment 20

[Jason Smith [:jsmith]]

koi+ for sec-critical bug

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c915f234da21
https://hg.mozilla.org/releases/mozilla-beta/rev/74e9dd9a411c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1b66574518e6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

The m-c bustage fix broke the other branches. Go figure.
https://hg.mozilla.org/releases/mozilla-aurora/rev/6800c2e02e51
https://hg.mozilla.org/releases/mozilla-beta/rev/3161b119617a
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ba278ef30b1e

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

With Nicholas' instructions, I reproduced the crash in FF28 from 2013-11-13. It took around 10 page refreshes to repro.

In each fixed branch, I allowed the page to refresh more than 30 times per run - no crash.

Confirmed fixed FF27, FF28 and FF29, 2014-01-18.