Closed Bug 978522 Opened 6 years ago Closed 6 years ago

Crash when nesting console.log toString

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox29 --- unaffected
firefox30 --- verified
firefox-esr24 --- unaffected

People

(Reporter: jruderman, Assigned: baku)

References

Details

(5 keywords)

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file testcase
No description provided.
Can I see the full backtrace?
#0  mozilla::dom::Console::ProcessArguments (this=<optimized out>, aCx=<optimized out>, aData=..., aSequence=...) at /home/smaug/mozilla/hg/inbound/dom/base/Console.cpp:1206
#1  0x00007ffff4bea322 in mozilla::dom::Console::ProcessCallData (this=0x7fffd24f7ec0, aData=...) at /home/smaug/mozilla/hg/inbound/dom/base/Console.cpp:949
#2  0x00007ffff4bea0c4 in mozilla::dom::Console::Notify (this=0x7fffd24f7ec0, timer=<optimized out>) at /home/smaug/mozilla/hg/inbound/dom/base/Console.cpp:895
#3  0x00007ffff3b681ca in nsTimerImpl::Fire (this=0x7fffdb14b830) at /home/smaug/mozilla/hg/inbound/xpcom/threads/nsTimerImpl.cpp:554
#4  0x00007ffff3b68378 in nsTimerEvent::Run (this=<optimized out>) at /home/smaug/mozilla/hg/inbound/xpcom/threads/nsTimerImpl.cpp:635
#5  0x00007ffff3b65d90 in nsThread::ProcessNextEvent (this=0x7ffff7ddfb60, mayWait=false, result=0x7fffffffc757) at /home/smaug/mozilla/hg/inbound/xpcom/threads/nsThread.cpp:643
#6  0x00007ffff3b09fc5 in NS_ProcessNextEvent (thread=<optimized out>, mayWait=false) at /home/smaug/mozilla/hg/inbound/xpcom/glue/nsThreadUtils.cpp:263
#7  0x00007ffff3db4486 in mozilla::ipc::MessagePump::Run (this=0x7fffec17c1c0, aDelegate=0x7ffff7d7f420) at /home/smaug/mozilla/hg/inbound/ipc/glue/MessagePump.cpp:95
#8  0x00007ffff3d8c1be in RunInternal (this=0x5a5a5a5a5a5a5a5a) at /home/smaug/mozilla/hg/inbound/ipc/chromium/src/base/message_loop.cc:226
#9  RunHandler (this=0x5a5a5a5a5a5a5a5a) at /home/smaug/mozilla/hg/inbound/ipc/chromium/src/base/message_loop.cc:219
#10 MessageLoop::Run (this=0x5a5a5a5a5a5a5a5a) at /home/smaug/mozilla/hg/inbound/ipc/chromium/src/base/message_loop.cc:193
#11 0x00007ffff4aa6e3e in nsBaseAppShell::Run (this=0x7fffe973e4e0) at /home/smaug/mozilla/hg/inbound/widget/xpwidgets/nsBaseAppShell.cpp:164
#12 0x00007ffff576aee3 in nsAppStartup::Run (this=0x7fffe954b150) at /home/smaug/mozilla/hg/inbound/toolkit/components/startup/nsAppStartup.cpp:276
#13 0x00007ffff56d5cbe in XREMain::XRE_mainRun (this=<optimized out>) at /home/smaug/mozilla/hg/inbound/toolkit/xre/nsAppRunner.cpp:3996
#14 0x00007ffff56d5f3d in XREMain::XRE_main (this=0x7fffffffcb50, argc=<optimized out>, argv=<optimized out>, aAppData=<optimized out>) at /home/smaug/mozilla/hg/inbound/toolkit/xre/nsAppRunner.cpp:4063
#15 0x00007ffff56d63bb in XRE_main (argc=-756395968, argv=0x7fffd24c2680, aAppData=0x7fffffffc1f8, aFlags=<optimized out>) at /home/smaug/mozilla/hg/inbound/toolkit/xre/nsAppRunner.cpp:4273
#16 0x00000000004041c9 in do_main (xreDirectory=0x7ffff7d2c540, argc=<optimized out>, argv=<optimized out>) at /home/smaug/mozilla/hg/inbound/browser/app/nsBrowserApp.cpp:282
#17 main (argc=<optimized out>, argv=<optimized out>) at /home/smaug/mozilla/hg/inbound/browser/app/nsBrowserApp.cpp:643
Console::Notify looks scary to me. mQueuedCalls may change while were processing
ProcessCallData(mQueuedCalls[i]);
Group: dom-core-security, core-security
I wonder if it was better to implement mQueuedCalls as LinkedList.
That way we wouldn't do memmoves or copies like currently when nsTArray increases.
Attached patch crash.patch (obsolete) — Splinter Review
Attachment #8384297 - Flags: review?(bugs)
Comment on attachment 8384297 [details] [diff] [review]
crash.patch

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Console)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mTimer)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mStorage)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
> 
>-  tmp->mQueuedCalls.Clear();
>+  while (ConsoleCallData* data = tmp->mQueuedCalls.popFirst()) {
>+    delete data;
>+  }
Since you do this in few places, could you perhaps add
ClearConsoleData() or some such helper to Console class

>-  for (uint32_t i = 0; i < tmp->mQueuedCalls.Length(); ++i) {
>-    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mQueuedCalls[i].mGlobal);
>+  for (ConsoleCallData* data = tmp->mQueuedCalls.getFirst(); data != nullptr;
>+       data = data->getNext()) {
>+    aCallbacks.Trace(&data->mGlobal, "data->mGlobal", aClosure);
Could you add JSVAL_IS_TRACEABLE also before this to be consistent.
(and to be safe is we end up re-traversing again after unlink.)



>     if (obs) {
>       obs->RemoveObserver(this, "inner-window-destroyed");
>     }
> 
>-    mQueuedCalls.Clear();
>+    while (ConsoleCallData* data = mQueuedCalls.popFirst()) {
>+      delete data;
>+    }
This could use the ClearConsoleData() helper

>@@ -845,141 +863,148 @@ Console::Method(JSContext* aCx, MethodNa
> 
>     ErrorResult rv;
>     nsRefPtr<nsPerformance> performance = win->GetPerformance(rv);
>     if (rv.Failed()) {
>       Throw(aCx, rv.ErrorCode());
>       return;
>     }
> 
>-    callData.mMonotonicTimer = performance->Now();
>+    callData->mMonotonicTimer = performance->Now();
>   }
> 
>+  // The operation is completed. RAII class has to be disabled.
>+  raii.Finished();
>+
>   if (!NS_IsMainThread()) {
>-    // Here we are in a worker thread.
>+    // Here we are in a worker thread. The ConsoleCallData has to been removed
>+    // from the list.
>+    mQueuedCalls.popLast();
Please add a comment that ConsoleCallDataRunnable will delete the object.


Make ConsoleCallDataRunnable::mCallData nsAutoPtr so that it is guaranteed to be deleted.
Call .forget() when passing to AppendCallData.
Attachment #8384297 - Flags: review?(bugs) → review-
And btw, I did test the patch and it seems to fix the crash :)
Attached patch crash.patchSplinter Review
Attachment #8384297 - Attachment is obsolete: true
Attachment #8384300 - Flags: review?(bugs)
Attachment #8384301 - Flags: review?(bugs)
Assignee: nobody → amarchesini
Comment on attachment 8384301 [details] [diff] [review]
First interdiff - about nsAutoPtr

You really want .forget() and not = nullptr;
= nullptr deletes the object.
Attachment #8384301 - Flags: review?(bugs) → review-
Comment on attachment 8384302 [details] [diff] [review]
Second interdiff - about all the rest

Ah, fixed here.
Attachment #8384302 - Flags: review?(bugs) → review+
Attachment #8384300 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/f05e05787c07
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Group: dom-core-security
QA Contact: kamiljoz
Before verification, reproduced the crash on OSX/Win using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-01-03-02-04-mozilla-central/
- Opened the above test case that was attached in comment #0 and fx instantly crashed

Once the issue was reproduced, used the following builds for verification:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-06-04-03-02-02-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-06-04-00-40-03-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/mac/en-US/

- Opened the test case using the builds mentioned above in both OSX 10.9.3 and Win 8.1
- Opened the test case on several tabs without issues
- Opened the test case in several e10s instances without issues
- Opened the test case in several Private Window instances without issues
Status: RESOLVED → VERIFIED
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.