ImageBridgeParent::RecvUpdate calls layers::Compositor::GetBackend from the wrong thread (e10s)

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bwc, Assigned: milan)

Tracking

({sec-high})

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox35 wontfix, firefox36+ fixed, firefox37+ fixed, firefox38+ fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main36-])

Attachments

(3 attachments, 2 obsolete attachments)

Just saw this stack while running some e10s mochitests under linux ASAN:

Jan 16 12:40:03 Assertion failure: CompositorParent::CompositorLoop() == MessageLoop::current() (Can only call this from the compositor thread!), at ../../../gfx/layers/Compositor.cpp:49
Jan 16 12:40:04 #01: mozilla::layers::ImageBridgeParent::RecvUpdate(nsTArray<mozilla::layers::CompositableOperation> const&, nsTArray<mozilla::layers::EditReply>*) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/gfx/layers/../../../gfx/layers/ipc/ ImageBridgeParent.cpp:120)                                                        

Jan 16 12:40:04 #02: mozilla::layers::ImageBridgeParent::RecvUpdateNoSwap(nsTArray<mozilla::layers::CompositableOperation> const&) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/gfx/layers/../../../gfx/layers/ipc/ImageBridgeParent.cpp:150)  
Jan 16 12:40:04 #03: mozilla::layers::PImageBridgeParent::OnMessageReceived(IPC::Message const&) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/ipdl/./PImageBridgeParent.cpp:384)
Jan 16 12:40:04 #04: ~AutoSetValue (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/glue/../../dist/include/mozilla/ipc/MessageChannel.h:490)
Jan 16 12:40:04 #05: ~Maybe (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/glue/../../dist/include/mozilla/Maybe.h:95)
Jan 16 12:40:04 #06: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/glue/../../../ipc/glue/MessageChannel.cpp:1130)
Jan 16 12:40:04 #07: MessageLoop::RunTask(Task*) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/message_loop.cc:361)
Jan 16 12:40:04 #08: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/message_loop.cc:369)
Jan 16 12:40:04 #09: MessageLoop::DoWork() (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/message_loop.cc:447)
Jan 16 12:40:04 #10: base::MessagePumpDefault::Run(base::MessagePump::Delegate*) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/message_pump_default.cc:34)
Jan 16 12:40:04 #11: MessageLoop::RunInternal() (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/message_loop.cc:234)
Jan 16 12:40:04 #12: ~AutoRunState (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/message_loop.cc:508)
Jan 16 12:40:04 #13: base::Thread::ThreadMain() (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/thread.cc:173)
Jan 16 12:40:04 #14: ThreadFunc(void*) (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/chromium/../../../ipc/chromium/src/base/platform_thread_posix.cc:39)
Jan 16 12:40:04 #15: ??? (/lib64/libpthread.so.0)                                                                 
Jan 16 12:40:04 #16: clone (/lib64/libc.so.6) 
Jan 16 12:40:04 #17: ??? (???:???)
Jan 16 12:40:04 ASAN:SIGSEGV                           

   Not sure how dangerous this really is, but marking security until the determination can be made.
Component: IPC → Graphics: Layers
> mozilla::layers::ImageBridgeParent::RecvUpdate(nsTArray<mozilla::layers::
> CompositableOperation> const&, nsTArray<mozilla::layers::EditReply>*)
> (/home/bcampen/checkouts/latest-central/objdir-ff-asan/gfx/layers/../../../
> gfx/layers/ipc/ ImageBridgeParent.cpp:120)                                  
> Jan 16 12:40:04 #02:
> mozilla::layers::ImageBridgeParent::RecvUpdateNoSwap(nsTArray<mozilla::
> layers::CompositableOperation> const&)
> (/home/bcampen/checkouts/latest-central/objdir-ff-asan/gfx/layers/../../../
> gfx/layers/ipc/ImageBridgeParent.cpp:150)  
> Jan 16 12:40:04 #03:
> mozilla::layers::PImageBridgeParent::OnMessageReceived(IPC::Message const&)
> (/home/bcampen/checkouts/latest-central/objdir-ff-asan/ipc/ipdl/./
> PImageBridgeParent.cpp:384)


This stuff only ever runs on the compositor thread which is the correct one, so my guess is that this is happening late in the shutdown sequence after a point where the pointer to the compositor thread in CompositorParent.cpp has been de-initialized already (see CompositorParent::ShutDown) which means CompositorParent::CompositorLoop() isn't returning the compositor's message loop.
Posted patch Allow resetting the back end? (obsolete) — Splinter Review
So, should we reset the back end to LAYERS_NONE on shutdown?
Attachment #8551907 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8551907 [details] [diff] [review]
Allow resetting the back end?

Review of attachment 8551907 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, though i'd like to understand why recently we got a wave of bugs related to gfx ipc being used after the shutdown of threads.
Ideally we would not have sBackend at all (this thing is a ticking bomb on windows where there are popups using basic layers while the rest uses accelerated layers).
Attachment #8551907 - Flags: feedback?(nical.bugzilla) → feedback+
Blocks: 1091322
Blocks: 1117650
Just to increase the awareness: this patch seems to fix the intermittent in bug 1091322. And bug 1117650 needs this bugfix here to land.

Here is the test run which I'm basing my statements on: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cabfcd658a3
Note: as the attached patch doesn't apply to m-c cleanly any more I left out one MOZASSERT for the above try run.
Comment on attachment 8551907 [details] [diff] [review]
Allow resetting the back end?

Since this helps bug 1091322, let's go for the review, rather than feedback, and I'll get a try run going.
Attachment #8551907 - Flags: review?(nical.bugzilla)
Attachment #8551907 - Attachment description: Allow resetting the back end? (wip) → Allow resetting the back end?
Carry :nical's f+.  Rebased, otherwise no changes.
Attachment #8551907 - Attachment is obsolete: true
Attachment #8551907 - Flags: review?(nical.bugzilla)
Attachment #8554673 - Flags: review?(nical.bugzilla)
Attachment #8554673 - Flags: feedback+
Attachment #8554673 - Flags: review?(nical.bugzilla) → review+
(In reply to Milan Sreckovic [:milan] from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=98496948a2a5

WinXP debug looks pretty angry :(
I'm going to mark this sec-high, because it sounds like it could be bad, but if it is only happening on shut down that will make it harder to exploit.  Adjust as desired.
Keywords: sec-high
Before bug 972891 is fixed, we have to (keep) stay(ing) away from asserting that the compositor doesn't change.  This just gets us back to where the original code was, only putting out the warning on OS X, and not asserting anywhere.

Re-running try on windows only https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec550e006124
Attachment #8554673 - Attachment is obsolete: true
Attachment #8555461 - Flags: review+
Much better \m/. Ready for sec-approval? :)
Assignee: nobody → milan
Comment on attachment 8555461 [details] [diff] [review]
Clean up the asserts in the compositor.  Carry r=nical

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, especially as the most critical part is mixed up with additional debugging changes.  It also only applies to the debug build.

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?

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not hard, and should just apply, but this is a debug build only issue, so I'm not convinced it's worth it.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.
Attachment #8555461 - Flags: sec-approval?
This goes back forever. I'll give it sec-approval+ but we should take it on Aurora, Beta, and ESR31 as well.
Attachment #8555461 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #13)
> This goes back forever. I'll give it sec-approval+ but we should take it on
> Aurora, Beta, and ESR31 as well.

Al, this is a debug change only - do we still want it on these other branches?  I don't mind, just clarifying what we usually do.
Keywords: checkin-needed
Whiteboard: [gfx-noted]
https://hg.mozilla.org/mozilla-central/rev/cf5abbbc9f49
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(In reply to Milan Sreckovic [:milan] from comment #14)
> (In reply to Al Billings [:abillings] from comment #13)
> > This goes back forever. I'll give it sec-approval+ but we should take it on
> > Aurora, Beta, and ESR31 as well.
> 
> Al, this is a debug change only - do we still want it on these other
> branches?  I don't mind, just clarifying what we usually do.

Oh. I missed that. Yeah, if it is debug only, I think it can just ride the trains and we don't need it on ESR31.
Actually, can we please take this on 36/37? Bug 1091322 has bitten us at inopportune times due to random changes in test chunking and it would be great to get this bug resolved for them as well to avoid issues with other backports.
Approval Request Comment

See comment 18.  Debug only.
Attachment #8557127 - Flags: review+
Attachment #8557127 - Flags: approval-mozilla-aurora?
Approval Request Comment
See comment 18.  Debug only.
Flags: needinfo?(milan)
Attachment #8557128 - Flags: review+
Attachment #8557128 - Flags: approval-mozilla-beta?
Attachment #8557127 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8557128 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main36-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.