Andrew McCreight [:mccr8] Reporter
	Description • 9 years ago

This is a fairly common IPC assertion crash on Nightly. We're inside mozilla::layout::PVsyncChild::OnMessageReceived(), which calls nsRefreshDriver::Tick(), which calls mozilla::a11y::NotificationController::WillRefresh(), which then calls mozilla::a11y::PDocAccessibleChild::SendTextChangeEvent() or SendHideEvent(), which tries to send a message. This is on the main thread in the content process. I'm not sure if the issue is the nested IPC or whether a11y IPC is supposed to run no another thread or what.

Here are some example crash reports:
https://crash-stats.mozilla.com/report/index/9aef00e4-cefb-476a-8cc3-525fd2150717
https://crash-stats.mozilla.com/report/index/74205d66-570b-49bf-84a0-8a9a72150717
https://crash-stats.mozilla.com/report/index/46415e7d-cede-4614-a630-d34b02150716
https://crash-stats.mozilla.com/report/index/30a44a30-bb8b-479f-89c1-f05bf2150716

	Trevor Saunders (:tbsaunde) Assignee
	Comment 1 • 9 years ago

So, my understanding is that it assumed WillRefresh will call js, so I believe it *should* be safe for a11y to do this.

In the last one of those crashes we crashed on the line calling AssertWorkerThread() so my guess would be some how part of the ipc code is running on the wrong thread (or at least thinks it is).

I'm not that familiar with ipc code maybe Billm has ideas?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 2 • 9 years ago

I think that the IPC channel for ipcDoc is probably dead at this point, so the thread assertion is probably a red herring. DocAccessibleChild (and DocAccessibleParent as well) need to override the ActorDestroy method and ensure that no IPC messages are sent after that point (using a flag or something). IPDL requires this.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 3 • 9 years ago

(In reply to Bill McCloskey (:billm) from comment #2)
> I think that the IPC channel for ipcDoc is probably dead at this point, so
> the thread assertion is probably a red herring. DocAccessibleChild (and
> DocAccessibleParent as well) need to override the ActorDestroy method and

DocAccessibleParent does.

> ensure that no IPC messages are sent after that point (using a flag or
> something). IPDL requires this.

I'm not really sure how this happens in normal operation, but its easy enough to deal with.  That is I could see this happening when we kill a child process, but otherwise I would expect that no events are fired on a DocAccessible after the point where we shut down its actor, but I might be wrong about that.

	Daniel Veditz [:dveditz]
	Comment 4 • 9 years ago

Bill: does the fatal assert save us from any bad consequences, or might there be other paths that would slip through with a reference to a dead object?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 5 • 9 years ago

The assertion helps, but I don't think it's a guarantee against exploitability. A hacker would just have to set the worker ID to the correct value, which is probably very easy to guess.

	Lorien Hu (:lsocks)
	Comment 7 • 9 years ago

Comment on attachment 8637437 [details] [diff] [review]
make sure we don't send an event to a destroyed ipc document

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

I'm not comfortable enough with IPDL workings to catch anything wrong with this.

	Carsten Book [:Tomcat]
	Comment 8 • 9 years ago

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

	Andrew McCreight [:mccr8] Reporter
	Comment 9 • 9 years ago

How exploitable do you think this will be? Is this just during shutdown when we're not running any user code or can it happen at other times? Should we backport this to other branches?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 10 • 9 years ago

(In reply to Andrew McCreight [:mccr8] from comment #9)
> How exploitable do you think this will be? Is this just during shutdown when
> we're not running any user code or can it happen at other times? Should we
> backport this to other branches?

It only matters with e10s, and we have / will disable a11y with e10s on all existing release branches so I don't see any point in backporting.

How exploitable is it? I'm not really sure, I suspect it only happens when tabs are closed so it may well be no content js is running, and can't force this to happen but I don't really know.

	Andrew McCreight [:mccr8] Reporter
	Comment 11 • 9 years ago

Thanks of the explanation.

	Andrew McCreight [:mccr8] Reporter
	Comment 12 • 9 years ago

Err... thanks for the explanation.