723465 - Investigate if ListBase<LC>::finalize should use deferred release

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

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

I was trying to understand how new nodelist bindings work and noticed that
release may happen in ::finalize. Is it possible that it may cause similar problems
as what bug 712448 caused?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Would it be possible to assert in NS_RELEASE that we're not inside a GC? Ben says that he is using NS_RELEASE from a finalizer in a safe way, so maybe we could have some sort of whitelisted version that doesn't do the assert.

Comment 3

[Peter Van der Beken [:peterv]]

Bug 736825 has a testcase.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

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

Will we need this for branches?

What are the security implications here?

Comment 6

[Johnny Stenback (:jst)]

Yes, we'll need this for branches, the code exists all the way back to esr, but I don't know for sure if it's enabled there or not.

AFAICT, the implications here is that we could potentially run arbitrary code while we're in GC, and that's sg:critical kinda bad.

Peter, please correct me if I'm wrong here...

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Peter, is there anything stopping this from landing?

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to Bill McCloskey (:billm) from comment #7)
> Peter, is there anything stopping this from landing?

Nope, thanks for reminding me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/917f69eb26ef

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 607161 [details] [diff] [review]
v1

[Approval Request Comment]
Regression caused by (bug #): bug 648801
User impact if declined: security bug (could cause scripts running during the GC)
Testing completed (on m-c, etc.): landed on inbound, has a crashtest
Risk to taking this patch (and alternatives if risky): low risk, we're just delaying destruction of object till after the GC is done
String changes made by this patch: None

Comment 10

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/917f69eb26ef

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 607161 [details] [diff] [review]
v1

[Triage Comment]
This appears to be a low risk sg:high/crit fix early in the cycle. Approved for Beta 13 and Aurora 14.

Comment 12

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

Verified in debug trunk build for 4/30. When I run the testcase from Bug 736825 on the 4/28 debug build, I get the assert and then this crash:

bp-38118b2a-0b0d-4932-938d-833442120430

On 4/30 debug trunk, I get no assert and no crash.

Comment 13

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-beta/rev/14d93414e972
https://hg.mozilla.org/releases/mozilla-aurora/rev/cfaabe5f04be

Comment 14

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

Is this going in for ESR?

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 607161 [details] [diff] [review]
v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:crit
User impact if declined: crashes
Fix Landed on Version: 13
Risk to taking this patch (and alternatives if risky): low risk, we're just delaying destruction of object till after the GC is done
String or UUID changes made by this patch: none

Comment 16

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/ac1b28490529

Comment 17

[u279076]

Verified fixed using the testcase in bug 736825 with:
 * Firefox 10.0.6esrpre 2012-07-11
 * Firefox 15.0a2 2012-07-11
 * Firefox 14.0b10
 * Firefox 13.0.1