1919809 - Console does not clear ArgumentData JS pointers in Unlink

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Tarek has some patches that are hitting assertions with this stack, on a worker CC:

#01: AssertNoGcThing(JS::GCCellPtr, char const*, void*) [xpcom/base/CycleCollectedJSRuntime.cpp:1485]
#02: TraceCallbackFunc::Trace(JS::Heap<JS::Value>, char const, void*) const [xpcom/base/nsCycleCollectorTraceJSHelpers.cpp:37]
#03: mozilla::dom::Console::cycleCollection::Trace(void*, TraceCallbacks const&, void*) [dom/console/Console.cpp:813]
#04: mozilla::CycleCollectedJSRuntime::AssertNoObjectsToTrace(void*) [xpcom/base/CycleCollectedJSRuntime.cpp:1494]
#05: nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3268]

Console::ArgumentData contains JS pointers. These are traced, but not unlinked. In general, this could lead to dangling JS pointers when combined with other bugs in cycle collected code. However, in the case of Console, it looks like DropJSObject() is not called until the destructor, and it isn't wrapper cached, so I think we'll keep holding these objects alive as long as the console is alive, so this probably isn't really a security problem, but I'm uncomfortable enough with this I'll leave it hidden and marked sec-low. It looks like this has been around since the first landing of this code in bug 1492011, which was 4 years ago. It is odd nobody has come across this before.

Comment 1

[Andrew McCreight [:mccr8]]

Okay, it looks like the issue is a little subtler than I thought. The unlink method DOES call Console::Shutdown(), which calls ClearStorage, which does clear the ArgumentData. However, it bails out early under some conditions, so maybe that's why we don't normally see this.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1919809 - Always clear mArgumentStorage in Console's Unlink.

Comment 3

[Andrew McCreight [:mccr8]]

Please confirm that this fixes the issue for you (and doesn't cause other horrible crashes...) when you get the chance, Tarek. Thanks.

Comment 4

[Tarek Ziadé (:tarek)]

I ran

./mach test toolkit/components/ml/tests/browser/browser_ml_engine.js --headless

with my patch, and hit that assertion failure. With the patch applied the test does not hit it

thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/681583efde2c
Always clear mArgumentStorage in Console's Unlink. r=smaug

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/681583efde2c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR128 approval when you get a chance. It grafts cleanly.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Bug 1919809 - Always clear mArgumentStorage in Console's Unlink.

Original Revision: https://phabricator.services.mozilla.com/D222803

Comment 9

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: possible security problems
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: It clears an array slightly earlier
• String changes made/needed: none
• Is Android affected?: yes

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/61069393c651