David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Description • 8 years ago

Here are the ones I've come across so far. They'll have to be audited and fixed.

Random asserts:
http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/gfx/layers/client/TextureClient.cpp#1027
http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/gfx/layers/client/TextureClient.cpp#1129

Warnings that should not assert:
http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/gfx/layers/RotatedBuffer.cpp#679

Errors that are not handled:
http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/gfx/layers/client/ClientLayerManager.h#419

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 1 • 8 years ago

With this patch and the ones from bug 1305361, we can pretty consistently crash the GPU process and have the content process survive.

	George Wright (:gw280) (needinfo me!)
	Comment 2 • 8 years ago

Comment on attachment 8795572 [details] [diff] [review]
remove-assertions

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

I should probably have uploaded my own patches which are basically the same as this, but I had replaced the gfxCriticalError() calls with gfxCriticalNote. Afaik, gfxCriticalNote will still log in about:support and crash reports, but will not assert. I think there's still value in having the errors logged if the GPU process goes away.

	George Wright (:gw280) (needinfo me!)
	Comment 7 • 8 years ago

Those were my patches; I'd erred more on the side of keeping asserts if we can, and keeping logging if we can.

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 8 • 8 years ago

Comment on attachment 8795584 [details] [diff] [review]
0003-Bug-1297568-Downgrade-an-IPC-channel-error-from-gfxC.patch

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

The IPC errors I'd rather just remove than downgrade. With the GPU process, we're assuming they can fail and we'll recover, so no need to have extra warning spam.

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 9 • 8 years ago

Comment on attachment 8795583 [details] [diff] [review]
0002-Bug-1297568-Don-t-begin-a-layer-transaction-if-our-I.patch

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

This seems reasonable but Matt knows this code better than I.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 10 • 8 years ago

Comment on attachment 8795583 [details] [diff] [review]
0002-Bug-1297568-Don-t-begin-a-layer-transaction-if-our-I.patch

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

This seems fine, but I don't think it actually gains us too much.

It would be nice if we had a return value here to indicate failure so that the caller could abandon painting entirely at this point. I understand not wanting to add that right now though.

I think we still need the change to ShadowLayers.cpp I had in my patch too btw.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 11 • 8 years ago

Comment on attachment 8795572 [details] [diff] [review]
remove-assertions

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

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +691,5 @@
>    }
>  
>    AutoTArray<Edit, 10> cset;
>    size_t nCsets = mTxn->mCset.size() + mTxn->mPaints.size();
> +  if (nCsets == 0 && !mTxn->RotationChanged()) {

Just explaining this change. mTxn is considered 'empty' if it has no mutants, no changesets and no paints. We return early from this function if the transaction is empty.

The code just before this return processes the mutants in the transaction, and adds a changeset (OpSetLayerAttributes) for each one.

The existing assertion here was checking that we now had at least one of (changesets + paints), since that should have been guaranteed by processing all mutants.

This process is now fallible (since we might have failed to create the Shadow for a layer), so the assertion can fail.

	Pulsebot
	Comment 12 • 8 years ago

Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e71083772164
Don't begin a layer transaction if our IPC channel is down r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/724705bf2482
Downgrade buffer creation failures if we're a reasonable size, and never assert r=dvander

	Pulsebot
	Comment 13 • 8 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d22a910d1b
Remove some invalid assertions that can happen when the GPU process crashes. r=dvander

	Carsten Book [:Tomcat]
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e71083772164
https://hg.mozilla.org/mozilla-central/rev/724705bf2482
https://hg.mozilla.org/mozilla-central/rev/e7d22a910d1b

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 15 • 8 years ago

Some of these assertions or crash reporter messages are very valid and required if we're *not* in the GPU world.  By removing them for everybody, we're missing information in the cases where we could use it.

Please revisit these and understand which ones are not required at all, and which ones are just not required when we're in the GPU process world.

For example, we lost a diagnostic message on a "180 crashes per week" open bug 1088300 because that message got removed in this bug.  Chances are, there are more like that.

Separate bug is fine, but this needs to be revisited.

	David Anderson [:dvander] - inactive, e-mail if emergency Reporter
	Comment 16 • 8 years ago

If checks are intended to be diagnostics, we should make sure to add a comment with a reference to the bug #. That would have made it more clear here. Anyway, I'll comment in bug 1088300.

	George Wright (:gw280) (needinfo me!)
	Comment 17 • 8 years ago

I've commented in bug 1088300 as well.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 18 • 8 years ago

It's more of a general conversation, rather than particular to that bug.  If the assert/error/whatever is OK *only* in the "we're using the GPU process" scenario, then we should treat it that way.