Closed
Bug 978522
Opened 11 years ago
Closed 11 years ago
Crash when nesting console.log toString
Categories
(Core :: DOM: Core & HTML, defect)
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Can I see the full backtrace?
Comment 2•11 years ago
|
||
#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•11 years ago
|
||
Console::Notify looks scary to me. mQueuedCalls may change while were processing
ProcessCallData(mQueuedCalls[i]);
Updated•11 years ago
|
Group: dom-core-security, core-security
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8384297 -
Flags: review?(bugs)
Assignee | ||
Comment 6•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5287fdb891d9
waiting for green on try.
Comment 7•11 years ago
|
||
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-
Comment 8•11 years ago
|
||
And btw, I did test the patch and it seems to fix the crash :)
Updated•11 years ago
|
status-firefox29:
--- → unaffected
status-firefox30:
--- → affected
status-firefox-esr24:
--- → unaffected
Keywords: csectype-uaf,
sec-critical
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8384297 -
Attachment is obsolete: true
Attachment #8384300 -
Flags: review?(bugs)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8384301 -
Flags: review?(bugs)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8384302 -
Flags: review?(bugs)
Updated•11 years ago
|
Assignee: nobody → amarchesini
tracking-firefox30:
--- → ?
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
Comment on attachment 8384302 [details] [diff] [review]
Second interdiff - about all the rest
Ah, fixed here.
Attachment #8384302 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #8384300 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 14•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a730ff677aed
pushed to try
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
tracking-firefox30:
? → ---
Updated•11 years ago
|
Group: dom-core-security
Updated•11 years ago
|
QA Contact: kamiljoz
Comment 17•10 years ago
|
||
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
Updated•9 years ago
|
Group: core-security
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•