1166585 - crash in mozilla::gfx::FilterWrappers::Crop

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

This bug was filed from the Socorro interface and is 
report bp-ab145313-1163-4608-852c-3f0372150520.
=============================================================

This is the second time I met this crash after upgrading to Firefox 38. The report for the first time is bp-8b398f92-4466-43bc-b484-ae24f2150516

It seems it happens when I closed the tab of Twitter, and then the browser area becomes black, and flash several time, and the icons in toolbars disappears, and finally it crashes.

Comment 1

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

Some D2DERR_RECREATE_TARGET errors, D2D1.1, sounds like TDR, Markus, should we protect against filter being nullptr here: http://hg.mozilla.org/releases/mozilla-release/annotate/62bee8cdd19f/gfx/src/FilterSupport.cpp#l151

Comment 2

[Markus Stange [:mstange]]

Hmm, this is really unfortunate. I don't want to require all callers of CreateFilter to handle nulls. Maybe we can, for the case that aDC->CreateEffect fails, create a dummy filter node that just makes everything render nothing?

Comment 3

[Markus Stange [:mstange]]

It looks like Crop isn't the only filter wrapper where we have crashes: https://crash-stats.mozilla.com/search/?signature=~FilterWrappers&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

mozilla::gfx::FilterWrappers::Crop             86  52.76 %
mozilla::gfx::FilterWrappers::ForSurface       55  33.74 %
mozilla::gfx::FilterWrappers::ToAlpha          11  6.75 %
mozilla::gfx::FilterWrappers::GaussianBlur     9   5.52 %
mozilla::gfx::FilterWrappers::SRGBToLinearRGB  1   0.61 %
mozilla::gfx::FilterWrappers::Unpremultiply    1   0.61 %

Comment 4

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

Bas, we have two options here - take what Markus is suggesting in comment 2 and change all CreateEffect (derived) methods to never fail/create dummy filter, in which case we should also update the 2D.h with a comment to that effect, or update 2D.h to be explicit about being able to return nullptr and have the callers always check.
Preferences?  Since this is a change (or not) to Moz2D.

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Bas, we have two options here - take what Markus is suggesting in comment 2
> and change all CreateEffect (derived) methods to never fail/create dummy
> filter, in which case we should also update the 2D.h with a comment to that
> effect, or update 2D.h to be explicit about being able to return nullptr and
> have the callers always check.
> Preferences?  Since this is a change (or not) to Moz2D.

I have a strong preference for returning nullptr and expecting users to check. This is how all the APIs work, it's unfortunate, but I believe the consistency is important.

Comment 6

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

I don't know if we need to more than protect against the nullptr return (we do in at least one place, but not in most), so let's do that work in this bug and if we need another one take it from there.

Kyle, this shouldn't be a problem to put together, can you take a look?  To be consistent and paranoid and redundant, get the HRESULT of the call, check it (e.g., https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeD2D1.cpp#550) as well as check the null pointer.

Markus, is gfxWarning() enough, or would you want to put a gfxCriticalError() when this fails?

Comment 7

[Markus Stange [:mstange]]

This does seem to happen quite a bit in practice, so gfxCriticalError() seems like a bit much. Maybe gfxCriticalErrorOnce()?

Comment 8

[Kyle Fung]

Attachment: Check for CreateFilter() returning NULL

Comment 9

[Markus Stange [:mstange]]

This is not enough, all callers of the FilterWrapper functions also need to handle null now. But we can consider it part 1 of many patches for this bug, if you want. :-)
By the way, Gecko style requires braces even around single line "if" bodies.

Comment 10

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

Comment on attachment 8611211 [details] [diff] [review]
Check for CreateFilter() returning NULL

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

We have a number of places where we CreateEffect non-filters (e.g., DrawTargetD2D1.cpp); it is probably worth a separate bug to decide if we want to be consistent and check the return values of all of those.  Kyle, can you create that bug?

::: gfx/2d/FilterNodeD2D1.cpp
@@ +549,5 @@
>  
>    hr = aDC->CreateEffect(GetCLDIDForFilterType(aType), byRef(effect));
>  
>    if (FAILED(hr)) {
> +    gfxCriticalErrorOnce() << "Failed to create effect for FilterType: " << hexa(hr);

I'd be paranoid and do
if (FAILED(hr) || !effect) {
}
even though we would not expect for a non-failure with null effect.  It reads a bit more explicit.

::: gfx/src/FilterSupport.cpp
@@ +128,5 @@
> +      transfer->SetAttribute(ATT_DISCRETE_TRANSFER_DISABLE_B, false);
> +      transfer->SetAttribute(ATT_DISCRETE_TRANSFER_TABLE_B, glinearRGBTosRGBMap, 256);
> +      transfer->SetAttribute(ATT_DISCRETE_TRANSFER_DISABLE_A, true);
> +      transfer->SetInput(IN_DISCRETE_TRANSFER_IN, aInput);
> +    }

The pattern we most commonly use in this case (for better or worse) is with the explicit nullptr return.  Still Markus' review, just mentioning it.

if (transfer) {
  transfer->SetAttribute
  ...
  return transfer.forget();
}
return nullptr;

Comment 11

[Kyle Fung]

Attachment: filter-null-check-v2.patch

It looks like the node pointers are only dereferenced in DrawFilter(), though I could be wrong about that.

Comment 12

[Kyle Fung]

Bug created to address checking ID2D1DeviceContext::CreateEffect calls:
Bug 1169039

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8611446 [details] [diff] [review]
filter-null-check-v2.patch

You're right, I had forgotten that the rest is as null-safe as it is.
So we only need some null checks in the DrawFilter implementations now.
For FilterNodeSoftware, you can can create a static void FilterNodeSoftware::DrawFilter(DrawTarget* aDT, FilterNode* aFilter, ...) method that includes the cast and the null-check, and then use that from all the DrawTarget*::DrawFilter implementations that currently call filterNodeSoftware->Draw().

Comment 14

[Kyle Fung]

Attachment: filter-null-check-v3.patch

Drawing a NULL filter can be avoided by checking it before calling DrawFilter() in RenderFilterDescription().

Comment 15

[Kyle Fung]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=249d2278973b

Comment 16

[Kyle Fung]

Attachment: filter-null-check-v4.patch

Moved portions of patch from Bug 1169039 to here.

Comment 17

[Kyle Fung]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1b43a36bb94

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7686d0443ab5

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/7686d0443ab5

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Should we uplift this bug to aurora or even beta?

Comment 21

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

Comment on attachment 8612456 [details] [diff] [review]
filter-null-check-v4.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Small number of crashes were observed.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Pretty much none :)  We stop a null pointer dereference.
[String/UUID change made/needed]:

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8612456 [details] [diff] [review]
filter-null-check-v4.patch

Less crash is better! Taking it in aurora.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Milan, is there any reason why you didn't request the uplift to beta?

Comment 24

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

Comment on attachment 8612456 [details] [diff] [review]
filter-null-check-v4.patch

Approval Request Comment

Fewer than 100 crashes in the past week, and late in the beta cycle - should we go for the beta uplift?  Making a request so that we can track the conversation about it.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Probably not than but Liz will make the call :)
Thanks.

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Less crash sounds great but I am worried about taking big changes that haven't had a lot of testing at this point. Unless it fixes a boatload of crashes in which case let's gamble.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 28

[Kyle Fung]

Attachment: filter-null-check-v4-aurora.patch

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2b196031bea

Comment 30

[Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows zero crashes after the fix landed in Firefox 40 and 41.

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=mozilla%3A%3Agfx%3A%3AFilterWrappers%3A%3ACrop#tab-reports