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

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.

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.

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.

(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.

Looks like the fix for this bug will have to be backed out, at least for now. Reopening.

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.

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

Nical, could you fill an uplift request for 32? Thanks.

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]:

STR from bug 1027103: * https://www.google.fr/maps/@45.522217,-122.681801,3a,90y,19h,90t/data=!3m4!1e1!3m2!1sNAhWh8lID_xtbpkI5COi_w!2e0!6m1!1e1 * Double Click on the map on the bottom left.

Backed out from Aurora for bustage. https://hg.mozilla.org/releases/mozilla-aurora/rev/78872d6478e3 https://tbpl.mozilla.org/php/getParsedLog.php?id=43949619&tree=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.

Still busted. Backed out again. https://hg.mozilla.org/releases/mozilla-aurora/rev/cc563253a046 https://tbpl.mozilla.org/php/getParsedLog.php?id=44035415&tree=Mozilla-Aurora

I'm so sorry. I forgot to qrefresh before uploading the patch :( this is the good version

Comment on attachment 8458619 [details] [diff] [review] patch that can be uplifted to aurora. (carrying r=jrmuizel) [Triage Comment] cf previous previous uplift request.

Verified fixed FF 33.0a1 (2014-07-21) Ubuntu 13.04 x64.

Verified as fixed using the following environment: FF 32.0b1 Build Id:20140722195627 OS:Ubuntu 13.0x x64

The bug is still present on FF 32 for Ubuntu 12.04 (Canonical's repository)

> 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.