786142 - (CVE-2012-4212) Heap-use-after-free in XPCWrappedNative::Mark

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Abhishek Arya]

This reproduces on trunk and hitting quite frequently with flaky test. I am trying to get a reliable testcase.

=================================================================
==24146== ERROR: AddressSanitizer heap-use-after-free on address 0x7f25472e02a0 at pc 0x7f2584cab0a4 bp 0x7fff7adad6b0 sp 0x7fff7adad6a8
READ of size 8 at 0x7f25472e02a0 thread T0
    #0 0x7f2584cab0a3 in XPCWrappedNative::Mark() const src/js/xpconnect/src/xpcprivate.h:2836
    #1 0x7f2584d22e14 in WrappedNativeMarker(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*) src/js/xpconnect/src/XPCWrappedNativeScope.cpp:433
    #2 0x7f258ff599d3 in JS_DHashTableEnumerate src/js/src/jsdhash.cpp:710
    #3 0x7f25848fd8ea in Native2WrappedNativeMap::Enumerate(JSDHashOperator (*)(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*), void*) src/js/xpconnect/src/XPCMaps.h:137
    #4 0x7f2584d22a14 in XPCWrappedNativeScope::MarkAllWrappedNativesAndProtos() src/js/xpconnect/src/XPCWrappedNativeScope.cpp:452
    #5 0x7f2584b4dd17 in XPCJSRuntime::FinalizeCallback(JSFreeOp*, JSFinalizeStatus, int) src/js/xpconnect/src/XPCJSRuntime.cpp:792
    #6 0x7f25900e6421 in BeginSweepPhase(JSRuntime*) src/js/src/jsgc.cpp:3779
    #7 0x7f25900e0cd0 in IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) src/js/src/jsgc.cpp:4214
    #8 0x7f25900de5a2 in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) src/js/src/jsgc.cpp:4392
    #9 0x7f2590093509 in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) src/js/src/jsgc.cpp:4500
    #10 0x7f2590083101 in js::GCSlice(JSRuntime*, js::JSGCInvocationKind, js::gcreason::Reason, long) src/js/src/jsgc.cpp:4538
    #11 0x7f258ffe027f in js::IncrementalGC(JSRuntime*, js::gcreason::Reason, long) src/js/src/jsfriendapi.cpp:171
    #12 0x7f2581df8e5e in nsJSContext::GarbageCollectNow(js::gcreason::Reason, nsJSContext::IsIncremental, nsJSContext::IsCompartment, nsJSContext::IsShrinking, long) src/dom/base/nsJSEnvironment.cpp:2922
    #13 0x7f2581e407fe in InterSliceGCTimerFired(nsITimer*, void*) src/dom/base/nsJSEnvironment.cpp:3204
    #14 0x7f2589762d22 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473
    #15 0x7f25897645d8 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556
    #16 0x7f25897279ce in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:624
    #17 0x7f25893c8e27 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #18 0x7f25881a5b2b in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:117
    #19 0x7f25899d4bd9 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #20 0x7f25899d4a22 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #21 0x7f25899d4907 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #22 0x7f258766f7ae in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #23 0x7f25862e0448 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:273
    #24 0x7f257cae79b0 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3800
    #25 0x7f257caedc24 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3877
    #26 0x7f257caf0cee in XRE_main src/toolkit/xre/nsAppRunner.cpp:3953
    #27 0x40c5bb in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #28 0x409e1f in main src/browser/app/nsBrowserApp.cpp:279
    #29 0x7f2599ba8c4d in ?? ??:0
0x7f25472e02a0 is located 32 bytes inside of 96-byte region [0x7f25472e0280,0x7f25472e02e0)
freed by thread T0 here:
    #0 0x4c3e30 in free ??:0
    #1 0x7f2596a46572 in moz_free src/memory/mozalloc/mozalloc.cpp:51
    #2 0x7f2584c6d245 in operator delete(void*) src/../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7f2584c72fc0 in XPCWrappedNative::Release() src/js/xpconnect/src/XPCWrappedNative.cpp:1214
    #4 0x7f2584c73bfe in XPCWrappedNative::FlatJSObjectFinalized() src/js/xpconnect/src/XPCWrappedNative.cpp:1338
    #5 0x7f2584d143c2 in WrappedNativeFinalize(js::FreeOp*, JSObject*, WNHelperType) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:617
    #6 0x7f2584cdd10c in XPC_WN_NoHelper_Finalize(js::FreeOp*, JSObject*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:623
    #7 0x7f2590118cd8 in JSObject::finalize(js::FreeOp*) src/js/src/jsobjinlines.h:235
    #8 0x7f25900feb6a in bool js::gc::FinalizeTypedArenas<JSObject>(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) src/js/src/jsgc.cpp:412
    #9 0x7f259006a0b9 in js::gc::FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) src/js/src/jsgc.cpp:449
    #10 0x7f259006bb91 in js::gc::ArenaLists::finalizeNow(js::FreeOp*, js::gc::AllocKind) src/js/src/jsgc.cpp:1626
    #11 0x7f259006b4d3 in js::gc::ArenaLists::queueObjectsForSweep(js::FreeOp*) src/js/src/jsgc.cpp:1722
    #12 0x7f25900e5db9 in BeginSweepPhase(JSRuntime*) src/js/src/jsgc.cpp:3750
    #13 0x7f25900e0cd0 in IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) src/js/src/jsgc.cpp:4214
    #14 0x7f25900de5a2 in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) src/js/src/jsgc.cpp:4392
    #15 0x7f2590093509 in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) src/js/src/jsgc.cpp:4500
    #16 0x7f2590083101 in js::GCSlice(JSRuntime*, js::JSGCInvocationKind, js::gcreason::Reason, long) src/js/src/jsgc.cpp:4538
    #17 0x7f258ffe027f in js::IncrementalGC(JSRuntime*, js::gcreason::Reason, long) src/js/src/jsfriendapi.cpp:171
    #18 0x7f2581df8e5e in nsJSContext::GarbageCollectNow(js::gcreason::Reason, nsJSContext::IsIncremental, nsJSContext::IsCompartment, nsJSContext::IsShrinking, long) src/dom/base/nsJSEnvironment.cpp:2922
    #19 0x7f2581e407fe in InterSliceGCTimerFired(nsITimer*, void*) src/dom/base/nsJSEnvironment.cpp:3204
    #20 0x7f2589762d22 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473
    #21 0x7f25897645d8 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556
    #22 0x7f25897279ce in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:624
    #23 0x7f25893c8e27 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #24 0x7f25881a5b2b in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:117
    #25 0x7f25899d4bd9 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #26 0x7f25899d4a22 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #27 0x7f25899d4907 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #28 0x7f258766f7ae in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #29 0x7f25862e0449 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:273
previously allocated by thread T0 here:
    #0 0x4c3ef0 in __interceptor_malloc ??:0
    #1 0x7f2596a466c6 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:57
    #2 0x7f2584c6555b in operator new(unsigned long) src/../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7f2584a98040 in XPCConvert::NativeInterface2JSObject(XPCLazyCallContext&, JS::Value*, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, unsigned int*) src/js/xpconnect/src/XPCConvert.cpp:920
    #4 0x7f2584a919be in XPCConvert::NativeData2JS(XPCLazyCallContext&, JS::Value*, void const*, nsXPTType const&, nsID const*, unsigned int*) src/js/xpconnect/src/XPCConvert.cpp:319
    #5 0x7f2584ad147b in XPCConvert::NativeData2JS(XPCCallContext&, JS::Value*, void const*, nsXPTType const&, nsID const*, unsigned int*) src/js/xpconnect/src/xpcprivate.h:3310
    #6 0x7f2584c97453 in CallMethodHelper::GatherAndConvertResults() src/js/xpconnect/src/XPCWrappedNative.cpp:2642
    #7 0x7f2584cf70c5 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1478
    #8 0x7f2590381951 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:372
    #9 0x7f259030ea41 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2413
    #10 0x7f2590275a35 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:309
    #11 0x7f259038ec66 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:494
    #12 0x7f2590390c1e in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:531
    #13 0x7f258fae8334 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5665
    #14 0x7f258faed2d1 in JS_EvaluateUCScriptForPrincipalsVersionOrigin src/js/src/jsapi.cpp:5746
    #15 0x7f2581e1162f in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1497
    #16 0x7f2581fc11bf in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9590
    #17 0x7f2581f78b39 in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9851
    #18 0x7f2581fbf21a in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10118
    #19 0x7f2589762d22 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473
    #20 0x7f25897645d8 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556
    #21 0x7f25897279ce in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:624
    #22 0x7f25893c8e27 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #23 0x7f25881a5595 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #24 0x7f25899d4bda in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:209
Shadow byte and word:
  0x1fe4a8e5c054: fd
  0x1fe4a8e5c050: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe4a8e5c030: 00 00 00 00 00 00 00 00
  0x1fe4a8e5c038: fb fb fb fb fb fb fb fb
  0x1fe4a8e5c040: fa fa fa fa fa fa fa fa
  0x1fe4a8e5c048: fa fa fa fa fa fa fa fa
=>0x1fe4a8e5c050: fd fd fd fd fd fd fd fd
  0x1fe4a8e5c058: fd fd fd fd fd fd fd fd
  0x1fe4a8e5c060: fa fa fa fa fa fa fa fa
  0x1fe4a8e5c068: fa fa fa fa fa fa fa fa
  0x1fe4a8e5c070: fd fd fd fd fd fd fd fd
Stats: 287M malloced (327M for red zones) by 649602 calls
Stats: 79M realloced by 89263 calls
Stats: 248M freed by 411873 calls
Stats: 117M really freed by 185135 calls
Stats: 532M (136282 full pages) mmaped in 133 calls
  mmaps   by size class: 8:442341; 9:40955; 10:16380; 11:14329; 12:3072; 13:1536; 14:1280; 15:256; 16:448; 17:1248; 18:208; 19:104; 20:20;
  mallocs by size class: 8:550715; 9:52926; 10:18484; 11:17824; 12:3165; 13:2149; 14:1685; 15:376; 16:564; 17:1361; 18:228; 19:106; 20:19;
  frees   by size class: 8:331303; 9:43136; 10:15020; 11:14592; 12:2157; 13:1812; 14:1407; 15:329; 16:504; 17:1346; 18:149; 19:103; 20:15;
  rfrees  by size class: 8:136366; 9:24837; 10:9558; 11:10277; 12:996; 13:829; 14:830; 15:183; 16:354; 17:869; 18:30; 19:5; 20:1;
Stats: malloc large: 1714 small slow: 2570
==24146== ABORTING

Comment 1

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

Thanks for the report. A test case for this would be great, even a flaky one.

Comment 2

[Abhishek Arya]

Attachment: Testcase

This kinda reduced testcase reproduces reliably on a fully symbolized asan release build (unoptimized) with multiple firefox instances (on my fast box like 17).

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Bill, Bobby, this seems bad... sec-critical? (Feel free to add keyword)

Comment 4

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

This seems to have something to do with a PreCreate hook returning an unexpected parent. When I run in a debug build, I usually get an assertion in MoveWrapper:

NS_ASSERTION(betterScope == newScope, "Weird scope returned");

After hitting this assertion, we always hit a crashing assert in the JS engine because we pass in a global object that doesn't match the one in the compartment.

The element that's being moved is always an HTML input element. The PreCreate hook for this thing seems to find the corresponding form and wrap that for the parent. I don't know why that would be in the wrong compartment, though.

I'm not sure if these assertions could cause the use-after-free issue above, but it seems pretty plausible.

Bobby, could you take a look at this?

Comment 5

[Daniel Veditz [:dveditz]]

qawanted: does this affect ESR-10 or Firefox 16? According to comment 4 you should be able to test with a debug build and not need an Asan build.

Comment 7

[Bobby Holley (:bholley)]

I got partway through debugging this a few days ago but was interrupted. It looks like a cross-compartment wrapper ends up in the parent chain of the input element somehow, which is wrong I think. I'll get back to it soon.

Comment 8

[Bobby Holley (:bholley)]

Oh boy this is tricky.

So, the input element is being reparented. It's parented to the document currently, which means that it wasn't inside a form at the time the XPCWN was instantiated. But since then, it has been added to a form, so the call to PreCreate during MoveWrapper returns a totally different parent (the form control) than was being used before. And _form_ is an orphan. But it's too late to fix it up, so we crash.

I still need to figure out how the form is becoming an orphan. But it leads me to wonder whether we really need to make this special case for form controls  during PreCreate. They're the only HTML element that isn't parented to the document, and thus the only content-accessible objects (I think) whose parents can become "out of date" in this sense due to regular DOM operations.

Peter or Boris, do you guys have any recollection of why we do this? Can we get rid of it?

Comment 9

[Boris Zbarsky [:bzbarsky]]

It's done because the scope chain for inline event handlers on those elements needs to include the form.

If we stop using the parent chain for scope chains, we can stop playing games with them here.

Comment 10

[u279076]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> qawanted: does this affect ESR-10 or Firefox 16? According to comment 4 you
> should be able to test with a debug build and not need an Asan build.

I was not able to reproduce any assertion with the attached testcase using the latest mozilla-esr10 and mozilla-central debug nightlies.

Comment 11

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #10)
> (In reply to Daniel Veditz [:dveditz] from comment #5)
> > qawanted: does this affect ESR-10 or Firefox 16? According to comment 4 you
> > should be able to test with a debug build and not need an Asan build.
> 
> I was not able to reproduce any assertion with the attached testcase using
> the latest mozilla-esr10 and mozilla-central debug nightlies.

Sorry, not mozilla-central, mozilla-beta. Just to clarify:

 * Firefox 10.0.8esrpre 2012-09-15 debug
 * Firefox 16.0 beta 2012-09-18 debug

Comment 12

[Bobby Holley (:bholley)]

I'm able to reproduce on m-c, and am debugging the issue. It's flakey, so it only fires ~1/5 runs.

That being said, I don't think we need help from QA at this point diagnosing which branches are affected. We'll have a pretty good idea by the end of it what's going on, and which branches that affects. I'll know more soon.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Crashtest. v1

Ok, wow. This is one tricky issue. This is probably the most complicated crashtest I've ever written. Ugh.

The gist of the issue here is that, with a lot of careful work, it's possible to cause a nasty
interaction between slim wrappers, multiple document.open() calls, orphan fixup, and input
form controls. Because slim wrappers don't get reparented during MoveWrappers() calls, it's possible
for a slim wrapper to be several scopes behind after multiple document.open calls. If one of
these wrappers then becomes the parent for an object being reparented in a subsequent document.open
call, it will be morphed and find itself in the wrong scope, causing a compartment mismatch.

I really, really, really want wrapper reparenting to just go away. It will with the new DOM bindings,
I believe (in particular, once everything is wrapper-cached.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Lowest-risk fix. v1

Here's a low-ish risk fix that's potentially backportable.

Comment 15

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=67e2dd27223e

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It will with the new DOM bindings, I believe (in particular, once everything is
> wrapper-cached.

I don't see how, given adoptNode....

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> I don't see how, given adoptNode....

When we no longer have XPCWN DOM objects, we won't need to sweep the entire scope on document.open, and a lot of the nastier parts of ReparentWrapperIfFound will go away, because we're no longer managing a separate data structure (XPCWrappedNative).

Comment 18

[Bobby Holley (:bholley)]

Attachment: Cleanup Part 1 - Explicitly reparent the document to the new scope during document.open. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Cleanup Part 2 - Be more aggressive when fixing up orphans. v1

To make stronger assumptions, we should dig deeper on the parent chain, and also morph any slim wrappers. This is slightly slower, but not much, since this stuff only gets called for HTML documents, and the parent chains there tend to be short. Moreover, this only gets called during document.open(), where performance doesn't matter so much.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Cleanup Part 3 - Replace MoveWrapper nonsense with orphan fixup. v1

Comment 21

[Bobby Holley (:bholley)]

I just attached another approach, probably landable only on trunk, where we simplify the work we do on document.open so that we just fix up orphans (no more calling PreCreate on each XPCWN). Pushing to try:

https://tbpl.mozilla.org/?tree=Try&rev=74a16e1bb0d5

Comment 22

[Bobby Holley (:bholley)]

Forgot to include some uncommitted changes in the last try push.

https://tbpl.mozilla.org/?tree=Try&rev=85d4ed567d1d

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 664049 [details] [diff] [review]
Lowest-risk fix. v1

The "kill MoveWrappers" approach has one orange, which I'll look into (I might split it into another bug). But the low-risk fix (which we'll need to backport) looks green. Flagging peter for review.

Comment 24

[Bobby Holley (:bholley)]

Though, Peter's pretty busy this week. Andrew, do you feel qualified to review this patch?

Comment 25

[Andrew McCreight [:mccr8]]

Sorry, I'm not really familiar with this part of the code.

Comment 26

[Daniel Veditz [:dveditz]]

Whether this affects 16 or not I think we can say it would be unwise to land xpconnect wrapper changes right before release. We should still get this fix into 17 ASAP.

Comment 27

[Bobby Holley (:bholley)]

I've filed bug 797304 for the high-risk version, since it's a reimplementation and doesn't point to this bug in particular.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 664049 [details] [diff] [review]
Lowest-risk fix. v1

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Somewhat. The code pretty straightforwardly checks if it encounters a slim wrapper during MoveWrappers and morphs it if so. But it also eliminates a similar check further down in the function, so it could also just look like moving code around.

That being said, actually writing a successful crashtest took me (the person most familiar with this arcane code) more than a full day (see comment 13).

Bug 797304 would be better (fixes the same-issue by just overhauling the code), but it's probably too risky to backport.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

FF15 and up.

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

CPG.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply everywhere.

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

Unlikely to cause regressions - pretty specific to the issue at hand.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 664049 [details] [diff] [review]
Lowest-risk fix. v1

This can go in any time, sooner is better to give time to flush out regressions if there are any. Don't forget to set the in-testsuite? flag or file a secondary bug so we don't forget to check in the crashtest once we've released the fix.

Comment 30

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a38038d6c58

Comment 31

[Bobby Holley (:bholley)]

Setting the in-testsuite? flag to remind us to check in the crashtest later on.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a38038d6c58

Based on all the patches above, I can't tell for sure if this should be resolved or not. Please do so if needed. And please mark patches as obsolete if they are.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen from comment #32)
> https://hg.mozilla.org/mozilla-central/rev/6a38038d6c58
> 
> Based on all the patches above, I can't tell for sure if this should be
> resolved or not. Please do so if needed. And please mark patches as obsolete
> if they are.

Sorry for the confusion. The other patches have moved to bug 797304.

Comment 34

[Bobby Holley (:bholley)]

Comment on attachment 664225 [details] [diff] [review]
Cleanup Part 3 - Replace MoveWrapper nonsense with orphan fixup. v1

># HG changeset patch
># User Bobby Holley <[e-mail]>
>
>Bug 786142 - Replace MoveWrapper nonsense with orphan fixup. v1
>
>diff --git a/content/base/public/nsContentUtils.h b/content/base/public/nsContentUtils.h
>index 2d3607f..ccf800a 100644
>--- a/content/base/public/nsContentUtils.h
>+++ b/content/base/public/nsContentUtils.h
>@@ -194,24 +194,16 @@ class nsContentUtils
> public:
>   static nsresult Init();
> 
>   /**
>    * Get a JSContext from the document's scope object.
>    */
>   static JSContext* GetContextFromDocument(nsIDocument *aDocument);
> 
>-  /**
>-   * When a document's scope changes (e.g., from document.open(), call this
>-   * function to move all content wrappers from the old scope to the new one.
>-   */
>-  static nsresult ReparentContentWrappersInScope(JSContext *cx,
>-                                                 nsIScriptGlobalObject *aOldScope,
>-                                                 nsIScriptGlobalObject *aNewScope);
>-
>   static bool     IsCallerChrome();
> 
>   static bool     IsCallerTrustedForRead();
> 
>   static bool     IsCallerTrustedForWrite();
> 
>   /**
>    * Check whether a caller has UniversalXPConnect.
>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp
>index 863f4b4..3baaf72 100644
>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>@@ -1708,33 +1708,16 @@ nsContentUtils::TraceSafeJSContext(JSTracer* aTrc)
>   if (!cx) {
>     return;
>   }
>   if (JSObject* global = JS_GetGlobalObject(cx)) {
>     JS_CALL_OBJECT_TRACER(aTrc, global, "safe context");
>   }
> }
> 
>-nsresult
>-nsContentUtils::ReparentContentWrappersInScope(JSContext *cx,
>-                                               nsIScriptGlobalObject *aOldScope,
>-                                               nsIScriptGlobalObject *aNewScope)
>-{
>-  JSObject *oldScopeObj = aOldScope->GetGlobalJSObject();
>-  JSObject *newScopeObj = aNewScope->GetGlobalJSObject();
>-
>-  if (!newScopeObj || !oldScopeObj) {
>-    // We can't really do anything without the JSObjects.
>-
>-    return NS_ERROR_NOT_AVAILABLE;
>-  }
>-
>-  return sXPConnect->MoveWrappers(cx, oldScopeObj, newScopeObj);
>-}
>-
> nsPIDOMWindow *
> nsContentUtils::GetWindowFromCaller()
> {
>   JSContext *cx = nullptr;
>   sThreadJSContextStack->Peek(&cx);
> 
>   if (cx) {
>     nsCOMPtr<nsPIDOMWindow> win =
>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
>index 982ed25..c23fc33 100644
>--- a/content/html/document/src/nsHTMLDocument.cpp
>+++ b/content/html/document/src/nsHTMLDocument.cpp
>@@ -1518,17 +1518,17 @@ nsHTMLDocument::Open(const nsAString& aContentTypeOrUrl,
>     if (oldScope && newScope != oldScope) {
>       nsCOMPtr<nsISupports> doc = do_QueryObject(this);
>       nsIXPConnect *xpc = nsContentUtils::XPConnect();
>       nsCOMPtr<nsIXPConnectJSObjectHolder> ignored;
>       rv = xpc->ReparentWrappedNativeIfFound(cx, oldScope->GetGlobalJSObject(),
>                                              newScope->GetGlobalJSObject(),
>                                              doc, getter_AddRefs(ignored));
>       NS_ENSURE_SUCCESS(rv, rv);
>-      rv = nsContentUtils::ReparentContentWrappersInScope(cx, oldScope, newScope);
>+      rv = xpc->RescueOrphansInScope(cx, oldScope->GetGlobalJSObject());
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>   }
> 
>   // Call Reset(), this will now do the full reset
>   Reset(channel, group);
>   if (baseURI) {
>     mDocumentBaseURI = baseURI;
>diff --git a/js/xpconnect/idl/nsIXPConnect.idl b/js/xpconnect/idl/nsIXPConnect.idl
>index 348a4a4..f55c065 100644
>--- a/js/xpconnect/idl/nsIXPConnect.idl
>+++ b/js/xpconnect/idl/nsIXPConnect.idl
>@@ -298,17 +298,17 @@ interface nsIXPCFunctionThisTranslator : nsISupports
> %{ C++
> // For use with the service manager
> // {CB6593E0-F9B2-11d2-BDD6-000064657374}
> #define NS_XPCONNECT_CID \
> { 0xcb6593e0, 0xf9b2, 0x11d2, \
>     { 0xbd, 0xd6, 0x0, 0x0, 0x64, 0x65, 0x73, 0x74 } }
> %}
> 
>-[uuid(bd300b18-1c34-4589-8285-23a12cc580ea)]
>+[uuid(5f51d65b-882f-449f-b53d-32a8ce91830e)]
> interface nsIXPConnect : nsISupports
> {
> %{ C++
>   NS_DEFINE_STATIC_CID_ACCESSOR(NS_XPCONNECT_CID)
> %}
> 
>     /**
>      * Initializes classes on a global object that has already been created.
>@@ -537,19 +537,17 @@ interface nsIXPConnect : nsISupports
>                               in nsIXPCFunctionThisTranslator aTranslator);
> 
>     nsIXPConnectJSObjectHolder
>     reparentWrappedNativeIfFound(in JSContextPtr aJSContext,
>                                  in JSObjectPtr  aScope,
>                                  in JSObjectPtr  aNewParent,
>                                  in nsISupports  aCOMObj);
>     void
>-    moveWrappers(in JSContextPtr aJSContext,
>-                 in JSObjectPtr  aOldScope,
>-                 in JSObjectPtr  aNewScope);
>+    rescueOrphansInScope(in JSContextPtr aJSContext, in JSObjectPtr  aScope);
> 
>     void clearAllWrappedNativeSecurityPolicies();
> 
>     nsIXPConnectJSObjectHolder
>     getWrappedNativePrototype(in JSContextPtr aJSContext,
>                               in JSObjectPtr  aScope,
>                               in nsIClassInfo aClassInfo);
> 
>diff --git a/js/xpconnect/src/nsXPConnect.cpp b/js/xpconnect/src/nsXPConnect.cpp
>index 59b605b..4383ab9 100644
>--- a/js/xpconnect/src/nsXPConnect.cpp
>+++ b/js/xpconnect/src/nsXPConnect.cpp
>@@ -1542,161 +1542,43 @@ MoveableWrapperFinder(JSDHashTable *table, JSDHashEntryHdr *hdr,
> 
>     // If a wrapper is expired, then there are no references to it from JS, so
>     // we don't have to move it.
>     if (!wn->IsWrapperExpired())
>         array->AppendElement(wn);
>     return JS_DHASH_NEXT;
> }
> 
>-static nsresult
>-MoveWrapper(XPCCallContext& ccx, XPCWrappedNative *wrapper,
>-            XPCWrappedNativeScope *newScope, XPCWrappedNativeScope *oldScope)
>-{
>-    // First, check to see if this wrapper really needs to be
>-    // reparented.
>-
>-    if (wrapper->GetScope() == newScope) {
>-        // The wrapper already got moved, nothing to do here.
>-        return NS_OK;
>-    }
>-
>-    // For performance reasons, we wait to fix up orphaned wrappers (wrappers
>-    // whose parents have moved to another scope) until right before they
>-    // threaten to confuse us.
>-    //
>-    // If this wrapper is an orphan, reunite it with its parent. If, following
>-    // that, the wrapper is no longer in the old scope, then we don't need to
>-    // reparent it.
>-    MOZ_ASSERT(wrapper->GetScope() == oldScope);
>-    nsresult rv = wrapper->RescueOrphans(ccx);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    if (wrapper->GetScope() != oldScope)
>-        return NS_OK;
>-
>-    nsISupports *identity = wrapper->GetIdentityObject();
>-    nsCOMPtr<nsIClassInfo> info(do_QueryInterface(identity));
>-
>-    // ClassInfo is implemented as singleton objects. If the identity
>-    // object here is the same object as returned by the QI, then it
>-    // is the singleton classinfo, so we don't need to reparent it.
>-    if (SameCOMIdentity(identity, info))
>-        info = nullptr;
>-
>-    if (!info)
>-        return NS_OK;
>-
>-    XPCNativeScriptableCreateInfo sciProto;
>-    XPCNativeScriptableCreateInfo sci;
>-    const XPCNativeScriptableCreateInfo& sciWrapper =
>-        XPCWrappedNative::GatherScriptableCreateInfo(identity, info,
>-                                                     sciProto, sci);
>-
>-    // If the wrapper doesn't want precreate, then we don't need to
>-    // worry about reparenting it.
>-    if (!sciWrapper.GetFlags().WantPreCreate())
>-        return NS_OK;
>-
>-    JSObject *newParent = oldScope->GetGlobalJSObject();
>-    rv = sciWrapper.GetCallback()->PreCreate(identity, ccx,
>-                                             newParent,
>-                                             &newParent);
>-    if (NS_FAILED(rv))
>-        return rv;
>-
>-    if (newParent == oldScope->GetGlobalJSObject()) {
>-        // The old scope still works for this wrapper. We have to
>-        // assume that the wrapper will continue to return the old
>-        // scope from PreCreate, so don't move it.
>-        return NS_OK;
>-    }
>-
>-    // The wrapper returned a new parent. If the new parent is in a
>-    // different scope, then we need to reparent it, otherwise, the
>-    // old scope is fine.
>-
>-    XPCWrappedNativeScope *betterScope =
>-        XPCWrappedNativeScope::FindInJSObjectScope(ccx, newParent);
>-    if (betterScope == oldScope) {
>-        // The wrapper asked for a different object, but that object
>-        // was in the same scope. This means that the new parent
>-        // simply hasn't been reparented yet, so reparent it first,
>-        // and then continue reparenting the wrapper itself.
>-
>-        if (!IS_WN_WRAPPER_OBJECT(newParent)) {
>-            // The parent of wrapper is a slim wrapper, in this case
>-            // we need to morph the parent so that we can reparent it.
>-
>-            NS_ENSURE_STATE(MorphSlimWrapper(ccx, newParent));
>-        }
>-
>-        XPCWrappedNative *parentWrapper =
>-            XPCWrappedNative::GetWrappedNativeOfJSObject(ccx, newParent);
>-
>-        rv = MoveWrapper(ccx, parentWrapper, newScope, oldScope);
>-        NS_ENSURE_SUCCESS(rv, rv);
>-
>-        // If the parent wanted to stay in the old scope, we have to stay with
>-        // it. This can happen when doing document.write when the old detached
>-        // about:blank document is still floating around in the scope. Leave it
>-        // behind to die.
>-        if (parentWrapper->GetScope() == oldScope)
>-            return NS_OK;
>-        NS_ASSERTION(parentWrapper->GetScope() == newScope,
>-                     "A _third_ scope? Oh dear...");
>-
>-        newParent = parentWrapper->GetFlatJSObject();
>-    } else
>-        NS_ASSERTION(betterScope == newScope, "Weird scope returned");
>-
>-    // Now, reparent the wrapper, since we know that it wants to be
>-    // reparented.
>-
>-    nsRefPtr<XPCWrappedNative> junk;
>-    rv = XPCWrappedNative::ReparentWrapperIfFound(ccx, oldScope,
>-                                                  newScope, newParent,
>-                                                  wrapper->GetIdentityObject(),
>-                                                  getter_AddRefs(junk));
>-    return rv;
>-}
>-
>-/* void moveWrappers(in JSContextPtr aJSContext, in JSObjectPtr  aOldScope, in JSObjectPtr  aNewScope); */
>+/* void rescueOrphansInScope(in JSContextPtr aJSContext, in JSObjectPtr  aScope); */
> NS_IMETHODIMP
>-nsXPConnect::MoveWrappers(JSContext *aJSContext,
>-                          JSObject *aOldScope,
>-                          JSObject *aNewScope)
>+nsXPConnect::RescueOrphansInScope(JSContext *aJSContext, JSObject *aScope)
> {
>     XPCCallContext ccx(NATIVE_CALLER, aJSContext);
>     if (!ccx.IsValid())
>         return UnexpectedFailure(NS_ERROR_FAILURE);
> 
>-    XPCWrappedNativeScope *oldScope =
>-        XPCWrappedNativeScope::FindInJSObjectScope(ccx, aOldScope);
>-    if (!oldScope)
>+    XPCWrappedNativeScope *scope =
>+        XPCWrappedNativeScope::FindInJSObjectScope(ccx, aScope);
>+    if (!scope)
>         return UnexpectedFailure(NS_ERROR_FAILURE);
> 
>-    XPCWrappedNativeScope *newScope =
>-        XPCWrappedNativeScope::FindInJSObjectScope(ccx, aNewScope);
>-    if (!newScope)
>-        return UnexpectedFailure(NS_ERROR_FAILURE);
>-
>-    // First, look through the old scope and find all of the wrappers that
>-    // we're going to move.
>+    // First, look through the old scope and find all of the wrappers that we
>+    // might need to rescue.
>     nsTArray<nsRefPtr<XPCWrappedNative> > wrappersToMove;
> 
>     {   // scoped lock
>         XPCAutoLock lock(GetRuntime()->GetMapLock());
>-        Native2WrappedNativeMap *map = oldScope->GetWrappedNativeMap();
>+        Native2WrappedNativeMap *map = scope->GetWrappedNativeMap();
>         wrappersToMove.SetCapacity(map->Count());
>         map->Enumerate(MoveableWrapperFinder, &wrappersToMove);
>     }
> 
>     // Now that we have the wrappers, reparent them to the new scope.
>     for (uint32_t i = 0, stop = wrappersToMove.Length(); i < stop; ++i) {
>-        nsresult rv = MoveWrapper(ccx, wrappersToMove[i], newScope, oldScope);
>+        nsresult rv = wrappersToMove[i]->RescueOrphans(ccx);
>         NS_ENSURE_SUCCESS(rv, rv);
>     }
> 
>     return NS_OK;
> }
> 
> /* void setSecurityManagerForJSContext (in JSContextPtr aJSContext, in nsIXPCSecurityManager aManager, in uint16_t flags); */
> NS_IMETHODIMP

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 664049 [details] [diff] [review]
Lowest-risk fix. v1

This patch is low risk. Flagging it for branch approval per dveditz's instructions in comment 29.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long ago.
User impact if declined: Potential compartment-mismatch.
Testing completed (on m-c, etc.): Baked on m-c several days
Risk to taking this patch (and alternatives if risky): low-risk
String or UUID changes made by this patch: none

Comment 36

[bhavana bajaj [:bajaj]]

Comment on attachment 664049 [details] [diff] [review]
Lowest-risk fix. v1

Approving for mozilla - aurora/beta as it is a low risk patch,baked on m-c for days and its better to get this in sooner in the cycle . (comment 29)

Comment 37

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/61cd70433751
https://hg.mozilla.org/releases/mozilla-beta/rev/46a7a1ccbfa0

Comment 38

[Bobby Holley (:bholley)]

Pushed the test.

Comment 39

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3cf0d570a8

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff3cf0d570a8

Comment 41

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old