Closed Bug 1166585 Opened 9 years ago Closed 9 years ago

crash in mozilla::gfx::FilterWrappers::Crop

Categories

(Core :: Graphics, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- verified
firefox41 --- verified

People

(Reporter: xidorn, Assigned: kyle_fung)

Details

(Keywords: crash, Whiteboard: gfx-noted)

Crash Data

Attachments

(2 files, 3 obsolete files)

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.
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
Flags: needinfo?(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?
Flags: needinfo?(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 %
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.
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
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?
Assignee: nobody → kfung
This does seem to happen quite a bit in practice, so gfxCriticalError() seems like a bit much. Maybe gfxCriticalErrorOnce()?
Attachment #8611211 - Flags: review?(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 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;
Attached patch filter-null-check-v2.patch (obsolete) — Splinter Review
It looks like the node pointers are only dereferenced in DrawFilter(), though I could be wrong about that.
Attachment #8611211 - Attachment is obsolete: true
Attachment #8611211 - Flags: review?(mstange)
Attachment #8611446 - Flags: review?(mstange)
Bug created to address checking ID2D1DeviceContext::CreateEffect calls:
Bug 1169039
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().
Attachment #8611446 - Flags: review?(mstange) → review+
Attached patch filter-null-check-v3.patch (obsolete) — Splinter Review
Drawing a NULL filter can be avoided by checking it before calling DrawFilter() in RenderFilterDescription().
Attachment #8611446 - Attachment is obsolete: true
Attachment #8612326 - Flags: feedback?(mstange)
Attachment #8612326 - Flags: feedback?(mstange) → review+
Moved portions of patch from Bug 1169039 to here.
Attachment #8612326 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7686d0443ab5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Should we uplift this bug to aurora or even beta?
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]:
Attachment #8612456 - Flags: approval-mozilla-aurora?
Comment on attachment 8612456 [details] [diff] [review]
filter-null-check-v4.patch

Less crash is better! Taking it in aurora.
Attachment #8612456 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Milan, is there any reason why you didn't request the uplift to beta?
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
Attachment #8612456 - Flags: approval-mozilla-beta?
Probably not than but Liz will make the call :)
Thanks.
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.
Attachment #8612456 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Needs rebasing for Aurora uplift.
Flags: needinfo?(kfung)
Flags: needinfo?(kfung)
You need to log in before you can comment on or make changes to this bug.