Closed Bug 1034593 Opened 5 years ago Closed 5 years ago

Large clips cause cairo to try to create huge mask surfaces, even when the source and destination are small.

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 + verified
firefox33 + verified
firefox-esr31 --- wontfix
relnote-firefox --- 31+

People

(Reporter: nical, Assigned: nical)

References

Details

(Whiteboard: STR in comment 20)

Attachments

(4 files, 3 obsolete files)

This causes us to fail miserably in google street view on linux (STR in bug 1019000).

Here is the relevant comment form bug 101900:

--

[...] there is a canvas2d (of size 845x762) that does a DrawImage with a very big clip (extents = { x:350, y:445, w:631559, y:379313}) which causes cairo to try to allocate an A8 mask of the size of the clip extents (and fail at it).

So we end up here: http://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-surface-fallback.c?from=_composite_trap_region#502

with a 845 by 762 destination surface (the canvas), an enormous clip path and a failed attempt at creating the mask.
(In reply to Nicolas Silva [:nical] from comment #0)
> This causes us to fail miserably in google street view on linux (STR in bug
> 1019000).

Sorry, I meant bug 1027103
Blocks: 1027103
This isn't easy to fix in cairo because it doesn't know how large the destination surface is. i.e. cairo_surface_t doesn't expose a size.
Attached patch terribly hacky fix (obsolete) — Splinter Review
As Jeff points out cairo_surface_t doesn't expose a size, so this patch gets around that by #including cairo-xlib-surface-private.h into cairo-clip.c which is bad, but it proves that it works. I'm not sure how to get this to work without this kind of hackiness.
A better solution.
Attachment #8450993 - Attachment is obsolete: true
Attachment #8451037 - Flags: review?(jmuizelaar)
Comment on attachment 8451037 [details] [diff] [review]
Teach cairo to not crate oversized masks if the clip extents are bigger than the destination surface

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

::: gfx/cairo/cairo/src/cairo-clip.c
@@ +959,5 @@
>      cairo_clip_path_t *prev;
>      cairo_status_t status;
> +    cairo_rectangle_int_t target_extents;
> +    int mw;
> +    int mh;

What does 'm' mean?

@@ +974,5 @@
> +    mw = clip_extents->width;
> +    mh = clip_extents->height;
> +    if (_cairo_surface_get_extents (target, &target_extents)) {
> +	mw = mw < target_extents.width ? mw : target_extents.width;
> +	mh = mh < target_extents.height ? mh : target_extents.height;

This should be target_extents.width + target_extents.x etc.

Maybe instead of doing this manually just have surface_extents that's set to target_extents and then intersect it with the clip extents and then replace the clip_extents pieces that follow with surface_extents.
Attachment #8451037 - Flags: review?(jmuizelaar) → review-
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 8451037 [details] [diff] [review]
> Teach cairo to not crate oversized masks if the clip extents are bigger than
> the destination surface
> 
> Review of attachment 8451037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/cairo/cairo/src/cairo-clip.c
> @@ +959,5 @@
> >      cairo_clip_path_t *prev;
> >      cairo_status_t status;
> > +    cairo_rectangle_int_t target_extents;
> > +    int mw;
> > +    int mh;
> 
> What does 'm' mean?

m for mask

> 
> Maybe instead of doing this manually just have surface_extents that's set to
> target_extents and then intersect it with the clip extents and then replace
> the clip_extents pieces that follow with surface_extents.

You are right, that's much better. Fixed in the next patch.
Attachment #8451602 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/2be56d24399f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1036029
Looks like the fix for this bug will have to be backed out, at least for now. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch BackoutSplinter Review
hg backout -r 192612:2be56d24399f && hg qnew cairo-backout

Pushing a clip at the CanvasRenderingContext2D level fixes the only place where we have run into this issue, so let's backout the cairo fix which is causing bug 1036029.
Attachment #8453847 - Flags: review?(jmuizelaar)
Attachment #8453850 - Flags: review?(jmuizelaar) → review+
Attachment #8453847 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8453850 [details] [diff] [review]
clip to the size of the canvas if the backend is cairo to avoid huge clip extents

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

It would be better if we actually neatly popped this clip as well.
(In reply to Bas Schouten (:bas.schouten) from comment #13)
> It would be better if we actually neatly popped this clip as well.

While forgetting to push the clip is mostly harmless as it is now, things would probably be riskier if we pop the clip somewhere else. Plus we'd have to handle the case of sErrorTarget. All in all it's not hard but I don't see the value. I don't feel strongly about it, so if you do, lets make it a followup.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d582915f484
(In reply to Nicolas Silva [:nical] from comment #15)
> (In reply to Bas Schouten (:bas.schouten) from comment #13)
> > It would be better if we actually neatly popped this clip as well.
> 
> While forgetting to push the clip is mostly harmless as it is now, things
> would probably be riskier if we pop the clip somewhere else. Plus we'd have
> to handle the case of sErrorTarget. All in all it's not hard but I don't see
> the value. I don't feel strongly about it, so if you do, lets make it a
> followup.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8d582915f484

Although backends should be garding for this, and I think currently none of them have issues with this. It's not completely unreasonable for a backend to leak data when a pushed clip isn't popped.
https://hg.mozilla.org/mozilla-central/rev/c3100f4255d1
https://hg.mozilla.org/mozilla-central/rev/8d582915f484
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Nical, could you fill an uplift request for 32? Thanks.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8453850 [details] [diff] [review]
clip to the size of the canvas if the backend is cairo to avoid huge clip extents

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: broken google streetview (black canvas) for linux and xp users if they use the map in the bottom-left.
[Describe test coverage new/current, TBPL]:
[Risks and why]: low risk, small change, easy to backout.
[String/UUID change made/needed]:
Attachment #8453850 - Flags: approval-mozilla-aurora?
Flags: needinfo?(nical.bugzilla)
Attachment #8453850 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
DrawTarget::GetType was renamed into GetBackendType but is still GetType on aurora.
Comment on attachment 8457932 [details] [diff] [review]
patch that can be uplifted to aurora. (carrying r=jrmuizel)

[Triage Comment]
cf previous uplift request.
Attachment #8457932 - Flags: approval-mozilla-aurora+
I'm so sorry. I forgot to qrefresh before uploading the patch :(
this is the good version
Attachment #8457932 - Attachment is obsolete: true
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8458619 [details] [diff] [review]
patch that can be uplifted to aurora. (carrying r=jrmuizel)

[Triage Comment]
cf previous previous uplift request.
Attachment #8458619 - Flags: approval-mozilla-aurora+
Verified fixed FF 33.0a1 (2014-07-21) Ubuntu 13.04 x64.
Whiteboard: STR in comment 20
Verified as fixed using the following environment:

FF 32.0b1
Build Id:20140722195627
OS:Ubuntu 13.0x x64
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 1050788
The bug is still present on FF 32 for Ubuntu 12.04 (Canonical's repository)
Duplicate of this bug: 1021703
> status-firefox31: --- → wontfix

Why not fix this on the ESR?
(In reply to Mike Hommey [:glandium] from comment #34)
> > status-firefox31: --- → wontfix
> 
> Why not fix this on the ESR?
Because we usually take only security patches in ESR or critical issues. 
This one was too risky for 31. However, since we didn't have any regression since then (afaik), maybe we could take it.
This does not meet ESR landing criteria, nor is it being requested by ESR users in the mailing list.
There's probably not a lot of Linux ESR users on the mailing list. OTOH it breaks streetview.
Still present on 33.0, Mozilla Firefox for Ubuntu, canonical - 1.0, installed right now.

$ uname -a
Linux ??? 3.13.0-37-generic #64~precise1-Ubuntu SMP Wed Sep 24 21:37:11 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

$ lsb_release -a
LSB Version:	core-2.0-amd64:core-2.0-noarch:core-3.0-amd64:core-3.0-noarch:core-3.1-amd64:core-3.1-noarch:core-3.2-amd64:core-3.2-noarch:core-4.0-amd64:core-4.0-noarch
Distributor ID:	Ubuntu
Description:	Ubuntu 12.04.5 LTS
Release:	12.04
Codename:	precise
An update. I verified that Street View in Google Maps' classic mode works, so maybe it worked in FF 32 too. I've been using the new Google Maps for the last few month and never tested with the old one. 

Even if I have a sensible workaround I can't say the bug has been resolved. Were the people that verified it fixed working in the new or in the classic Google Maps? Thanks.
Duplicate of this bug: 1128860
You need to log in before you can comment on or make changes to this bug.