1147249 - Fix crashes when a wheel transaction has a null confirmed apzc.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

If we confirm an InputBlock's target as null, and the input block is a wheel transaction, we can crash pretty easily. We assume that a wheel transaction always has a valid target.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8582826 [details] [diff] [review]
fix

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

I'd also like more MOZ_ASSERTs in WheelBlockState around places GetTargetApzc() is called. For example just below [1] add:
 MOZ_ASSERT(apzc); // Since we're in a transaction apzc must be non-null
Ditto for [2] and [3]. At [4] and [5] you can also add a MOZ_ASSERT(InTransaction());

Those were the obvious ones but feel free to add more if you think of any good places.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=78a291fd62b3#198
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=78a291fd62b3#268
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=78a291fd62b3#309
[4] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=78a291fd62b3#278
[5] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp?rev=78a291fd62b3#320

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

for the record not a fan of assert-not-null. it clutters up code when null derefs are easy to diagnose. but it seems to be allowed outside of js/src.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

s/allowed/common

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Anderson [:dvander] from comment #4)
> for the record not a fan of assert-not-null. it clutters up code when null
> derefs are easy to diagnose. but it seems to be allowed outside of js/src.

null derefs are easy to diagnose, yes. But adding assertions makes assumptions/constraints more obvious when you *write* the code so that you never even get null derefs to diagnose. The specific thing I wanted to document was that if we are in a transaction, the target APZC is guaranteed to be non-null. This is not readily obvious in different functions, hence my request for the assertion (and comment to go with it).

Once we have documented this assumption, then we can use the stronger MOZ_ASSERT(InTransaction()) in some places, which automatically implies MOZ_ASSERT(GetTargetApzc()). That's why I wanted you to assert InTransaction() in OnMouseMove and MaybeTimeout, since it implies GetTargetApzc() is non-null and also documents preconditions for calling those methods.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8583661 [details] [diff] [review]
v2

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

Clearing review for now; please update as per comment 2, specifically the InTransaction() asserts. I can live without the comment on the assertions but that would be nice too.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

I still don't understand why we'd document that pointers we're about to deref are valid to deref. Why not just assert InTransaction in the first place?

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Oh, I see you recommended that - I'll just do that everywhere.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Anderson [:dvander] from comment #8)
> I still don't understand why we'd document that pointers we're about to
> deref are valid to deref.

It was more so about documenting *why* they should be valid. In comment 2 I said to add:

MOZ_ASSERT(apzc); // Since we're in a transaction apzc must be non-null

Maybe I should have just had you add the comment, since that's the more important bit.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1147249.patch

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8583996 [details] [diff] [review]
bug1147249.patch

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

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +365,5 @@
> +    return false;
> +  }
> +
> +  MOZ_ASSERT(GetTargetApzc());
> +  return true;

Ah, that's a good idea. Thanks!

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb063ad596fd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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