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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Nicolas Silva [:nical]]

> 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.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Allow resetting the back end?

So, should we reset the back end to LAYERS_NONE on shutdown?

Comment 3

[Nicolas Silva [:nical]]

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).

Comment 4

[Nils Ohlmeier [:drno]]

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 5

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98496948a2a5

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Clean up the asserts in the compositor. r=nical

Carry :nical's f+.  Rebased, otherwise no changes.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(In reply to Milan Sreckovic [:milan] from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=98496948a2a5

WinXP debug looks pretty angry :(

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Clean up the asserts in the compositor. Carry r=nical

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Much better \m/. Ready for sec-approval? :)

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

This goes back forever. I'll give it sec-approval+ but we should take it on Aurora, Beta, and ESR31 as well.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

(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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Looks like the esr tracking flags got on the wrong version, btw.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Al Billings [:abillings - ex-MoCo]]

(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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Clean up the asserts in the compositor. Carry r=nical. Aurora/37 patch.

Approval Request Comment

See comment 18.  Debug only.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Clean up the asserts in the compositor. Carry r=nical. Beta/36 patch.

Approval Request Comment
See comment 18.  Debug only.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af25a4300b47
https://hg.mozilla.org/releases/mozilla-beta/rev/1ec36b2a9775