978522 - Crash when nesting console.log toString

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Andrea Marchesini [:baku]]

Can I see the full backtrace?

Comment 2

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

#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

Comment 3

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

Console::Notify looks scary to me. mQueuedCalls may change while were processing
ProcessCallData(mQueuedCalls[i]);

Comment 4

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

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 6

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=5287fdb891d9
waiting for green on try.

Comment 7

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

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.

Comment 8

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

And btw, I did test the patch and it seems to fix the crash :)

Comment 9

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 10

[Andrea Marchesini [:baku]]

Attachment: First interdiff - about nsAutoPtr

Comment 11

[Andrea Marchesini [:baku]]

Attachment: Second interdiff - about all the rest

Comment 12

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

Comment on attachment 8384301 [details] [diff] [review]
First interdiff - about nsAutoPtr

You really want .forget() and not = nullptr;
= nullptr deletes the object.

Comment 13

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

Comment on attachment 8384302 [details] [diff] [review]
Second interdiff - about all the rest

Ah, fixed here.

Comment 14

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=a730ff677aed
pushed to try

Comment 15

[Andrea Marchesini [:baku]]

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

Comment 16

[Ed Morley [:emorley]]

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

Comment 17

[Kamil Jozwiak [:kjozwiak]]

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