If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Canvas fillRect rendering fails with negative width or height beyond canvas boundary

RESOLVED FIXED in Firefox 43

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: M, Assigned: lsalzman)

Tracking

32 Branch
mozilla43
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; Tablet PC 2.0; rv:11.0) like Gecko

Steps to reproduce:

HTML:
<canvas id="test" width="200" height="200"></canvas>
JAVASCRIPT:
var canvas=document.getElementById('test');
var ctx = canvas.getContext("2d");
ctx.fillStyle = 'green';
ctx.fillRect(150, 150, -151, -110); // this fails to render as left edge is off canvas
ctx.fillRect(-1, 150, 151, -110); // this is equivalent and renders
// however
ctx.rect(150, 150, -151, -110);
ctx.fill(); // works as expected


ctx.fillRect(150, 150, -151, -110); // this fails to render



Actual results:

fillRect with negative widths or heights which take the edge 'off canvas' do not render


Expected results:

The fillRect should have rendered (chrome and IE render OK)

Comment 1

3 years ago
Can you attach a minimal self-contained testcase, please.
Flags: needinfo?(shophlf)
Created attachment 8497410 [details]
Working testcase based on comment #0

The testcase in comment #0 (attached) wfm (looks identical in Chrome and Firefox, anyway)
Attachment #8497410 - Attachment mime type: text/plain → text/html
And so does this:

data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<html lang%3D"en">%0D%0A <head>%0D%0A  <title>Test<%2Ftitle>%0D%0A <%2Fhead>%0D%0A <body>%0D%0A<canvas id%3D"test" width%3D"200" height%3D"200"><%2Fcanvas>%0D%0A<script>%0D%0Avar canvas%3Ddocument.getElementById('test')%3B%0D%0Avar ctx %3D canvas.getContext("2d")%3B%0D%0Actx.fillStyle %3D 'green'%3B%0D%0Actx.fillRect(150%2C 150%2C -151%2C -110)%3B %2F%2F this fails to render as left edge is off canvas%0D%0A<%2Fscript>%0D%0A <%2Fbody>%0D%0A<%2Fhtml>%0D%0A

Tempted to mark this WFM.

Comment 4

3 years ago
@Gijs: I see a small artifact with your testcase https://bugzilla.mozilla.org/attachment.cgi?id=8497410 on Nightly. Do you see it too? http://i.imgur.com/2uqIFIb.png
(In reply to Loic from comment #4)
> @Gijs: I see a small artifact with your testcase
> https://bugzilla.mozilla.org/attachment.cgi?id=8497410 on Nightly. Do you
> see it too? http://i.imgur.com/2uqIFIb.png

I don't, neither on OS X, nor on my Windows 8 machine. Might be GPU-specific?
Component: Untriaged → Canvas: 2D
Product: Firefox → Core

Comment 6

3 years ago
Yep, it disappears with HWA off, and I got a regression range.
(In reply to Loic from comment #6)
> Yep, it disappears with HWA off, and I got a regression range.

Great, can you file a different bug? :-)

Comment 8

3 years ago
Yes, but I'm going to update my GPU drivers before that, maybe it will fix it.

Comment 9

3 years ago
I filed bug 1074810.
(Reporter)

Comment 10

3 years ago
Created attachment 8497511 [details]
test case to show bug
Flags: needinfo?(shophlf)
(Reporter)

Comment 11

3 years ago
Hi
Test case test.html added

(In reply to Loic from comment #1)
> Can you attach a minimal self-contained testcase, please.
This testcase, too, renders the same on Fx 33 on OS X, compared to Chrome. I see 3 colored swatches (green, red, blue). I see the same on Firefox 33 on Windows.

What version of Firefox are you using? Can you post the graphics output from Help > Troubleshooting information? Does the problem happen if you restart in safe mode?

(I suspect GPU-acceleration is messing with things here)
Flags: needinfo?(shophlf)
(Reporter)

Comment 13

3 years ago
Application Basics
------------------

Name: Firefox
Version: 32.0.3
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:32.0) Gecko/20100101 Firefox/32.0

Crash Reports for the Last 3 Days
---------------------------------

All Crash Reports

Extensions
----------

Name: FiddlerHook
Version: 2.4.6.0
Enabled: false
ID: fiddlerhook@fiddler2.com

Graphics
--------

Adapter Description: NVIDIA GeForce FX 5200
Adapter Drivers: nvd3dum
Adapter RAM: 128
Device ID: 0x0322
DirectWrite Enabled: false (6.2.9200.16492)
Driver Date: 10-9-2006
Driver Version: 9.6.8.5
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 9
Vendor ID: 0x10de
WebGL Renderer: Google Inc. -- ANGLE (NVIDIA GeForce FX 5200 Direct3D9Ex vs_2_0 ps_2_0)
windowLayerManagerRemote: false
AzureCanvasBackend: skia
AzureContentBackend: cairo
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0

Important Modified Preferences
------------------------------

browser.cache.disk.capacity: 358400
browser.cache.disk.smart_size.first_run: false
browser.cache.disk.smart_size.use_old_max: false
browser.cache.frecency_experiment: 3
browser.places.smartBookmarksVersion: 7
browser.sessionstore.upgradeBackup.latestBuildID: 20140923175406
browser.startup.homepage_override.buildID: 20140923175406
browser.startup.homepage_override.mstone: 32.0.3
extensions.lastAppVersion: 32.0.3
network.cookie.prefsMigrated: true
places.database.lastMaintenance: 1412017075
places.history.expiration.transient_current_max_pages: 94629
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
privacy.sanitize.migrateFx3Prefs: true
storage.vacuum.last.index: 1
storage.vacuum.last.places.sqlite: 1411927526

JavaScript
----------

Incremental GC: true

Accessibility
-------------

Activated: false
Prevent Accessibility: 0

Library Versions
----------------

NSPR
Expected minimum version: 4.10.6
Version in use: 4.10.6

NSS
Expected minimum version: 3.16.5 Basic ECC
Version in use: 3.16.5 Basic ECC

NSSSMIME
Expected minimum version: 3.16.5 Basic ECC
Version in use: 3.16.5 Basic ECC

NSSSSL
Expected minimum version: 3.16.5 Basic ECC
Version in use: 3.16.5 Basic ECC

NSSUTIL
Expected minimum version: 3.16.5
Version in use: 3.16.5

Experimental Features
---------------------
(In reply to :Gijs Kruitbosch from comment #12)
> This testcase, too, renders the same on Fx 33 on OS X, compared to Chrome. I
> see 3 colored swatches (green, red, blue). I see the same on Firefox 33 on
> Windows.
> 
> What version of Firefox are you using? Can you post the graphics output from
> Help > Troubleshooting information? Does the problem happen if you restart
> in safe mode?
> 
> (I suspect GPU-acceleration is messing with things here)
Flags: needinfo?(shophlf)
Right, so if you restart Firefox in safe mode (Help > Restart with add-ons disabled) - do you still see the same issue?
Flags: needinfo?(shophlf)
(safe mode, contrary to the help menu label, will also disable hardware acceleration)
Component: Canvas: 2D → Graphics
(Reporter)

Comment 16

3 years ago
Hi
Yes I do.
I've also seen this on a friend's laptop (Fx 32 under W8) (which is why I investigated it).
Also checked on a W8 pc with fx 32.0.3.
Am I the only one seeing this (on 3 separate pcs)?

I note that I'm on 32.0.3 (which it tells me is up to date) but there's a ref to V33 above...

(In reply to :Gijs Kruitbosch from comment #14)
> Right, so if you restart Firefox in safe mode (Help > Restart with add-ons
> disabled) - do you still see the same issue?
Flags: needinfo?(shophlf)

Comment 17

3 years ago
Maybe you could attach a screenshot of what you're seeing, because we are not able to reproduce it.
(In reply to M from comment #16)
> Hi
> Yes I do.
> I've also seen this on a friend's laptop (Fx 32 under W8) (which is why I
> investigated it).
> Also checked on a W8 pc with fx 32.0.3.
> Am I the only one seeing this (on 3 separate pcs)?

Probably not, but we should figure out what you're seeing (see Loic's suggestion - a screenshot of what you expect (chrome/IE) and what you do see would be helpful!)

> I note that I'm on 32.0.3 (which it tells me is up to date) but there's a
> ref to V33 above...

33 is in beta (https://beta.mozilla.org/ ). I checked 32 on OS X just now, and that produces the same rendering for me. Could be that it's Windows-specific and got fixed in 33. I'm checking against 32 on Windows in a bit. Either way, screenshots would be useful :-)
(it looks the same for me on 32.0.3 on my win8 machine, I'm afraid)
(Reporter)

Comment 20

3 years ago
OK - I've uploaded a snip from both IE and firefox
M
(In reply to Loic from comment #17)
> Maybe you could attach a screenshot of what you're seeing, because we are
> not able to reproduce it.
(Reporter)

Comment 21

3 years ago
Created attachment 8497656 [details]
Output from Firefox and IE

Comment 22

3 years ago
I see you're running Windows 7 32 bits with an old GPU (NVIDIA GeForce FX 5200). Which drivers did you install? These ones for Vista?

The last drivers for your GPU on Win XP are 175.19 (23.6.2008). Are they compatible with Win 7?
(Reporter)

Comment 23

3 years ago
I think my setup is a red herring.
I've seen this on 3 totally different pc's, some new, some old, XP, W7, W8. In fact all pc's I have access to.
I really support open sources and the communities behind them - hence my researching this bug to find the cause (fillRect) and reporting it here.
I have a fix for my s/w (rect() then fill() rather than fillRect) so I'm OK.
If you can't reproduce it I suggest you drop it.
I think I've done all I can.
Good luck
M
(In reply to Loic from comment #22)
> I see you're running Windows 7 32 bits with an old GPU (NVIDIA GeForce FX
> 5200). Which drivers did you install? These ones for Vista?
> 
> The last drivers for your GPU on Win XP are 175.19 (23.6.2008). Are they
> compatible with Win 7?

Comment 24

2 years ago
I can reproduce the problem on Mac OS X 10.9.5 with both Firefox stable 40.0.3 and the developer edition 42.0a2 (2015-09-08). 

The same happens on Mac OS X 10.10 and Windows 7 running on VMWare Fusion 7 (tested with FF 32, 40.0.3 and the developer edition 42.0a2 (2015-09-08)).
Surprisingly, 11 months on, this reproduces trivially on my OSX machine (2013 15" retina MBP, NVIDIA GeForce GT 650M 1024 MB). I expect there is some difference in acceleration/gfx architecture that wasn't enabled for me before that is enabled now. Either way, um, Milan, can you have a look / prioritize this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(milan)

Comment 26

2 years ago
Our use case does not include strokeRect, but it seems the same problem affects it as well. Should a separate report be opened?
strokeRect would be the same root cause, we don't need a separate bug.

It reproduces because we switched OS X to use Skia for canvas (which is what unaccelerated Windows use as well.)  And Skia's SkCanvas::quickReject doesn't like this scenario.  If you were to set preference gfx.canvas.azure.backends to cairo, the problem would go away (not recommending this, just as a way to confirm this is a Skia issue.)

Lee, it may be worth asking Skia upstream how they'd feel about a change to SkCanvas::quickReject to support left > right and bottom > top cases.
Flags: needinfo?(milan)

Comment 28

2 years ago
Setting the preference gfx.canvas.azure.backends to cairo makes the problem disappear for both fillRect and strokeRect.
(Assignee)

Comment 29

2 years ago
Created attachment 8658935 [details] [diff] [review]
part 1 - Normalize Canvas 2D rects to be positive by flipping negative width or height.

The canvas spec allows rects to be specified with negative width or height, but both our Rect object and Skia's SkRect object do not support such rectangles.

Upon inspection of Chromium's canvas 2d code, we decided it made most sense to normalize the rects at the level of Canvas2DRenderingContext code rather than within the DrawTargets. Since the Rect passed in is technically empty, as far as our own code is concerned, it could cause bustage elsewhere if propagated downstream, so it is best to fix it upstream before this occurs.

Thus, this patch fixes the entry points for clearRect, fillRect, strokeRect, and drawImage to normalize any such negative dimensions before invalid Rect objects would have been created for them.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8658935 - Flags: review?(jmuizelaar)
(Assignee)

Comment 30

2 years ago
Created attachment 8658937 [details] [diff] [review]
part 2 - Add reftest to verify canvas 2d rects with negative dimensions are rendered properly

This adds a reftest based on the reporter's submitted test case, with some simplifications.

It just verifies that the rects supplied with negative dimensions render the same as rects that have positive dimensions but occupying the same space.
Attachment #8658937 - Flags: review?(jmuizelaar)
Attachment #8658937 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 31

2 years ago
Created attachment 8659353 [details] [diff] [review]
part 3 - remove failure meta-data for WPT tests with canvas drawImage with negative rects

Now that rects are normalized to be positive in 2d canvas, WPT tests are passing that weren't passing before, so failure meta-data needs to be removed.
Attachment #8659353 - Flags: review?(james)
Comment on attachment 8659353 [details] [diff] [review]
part 3 - remove failure meta-data for WPT tests with canvas drawImage with negative rects

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

Assuming this all passes try.

Note that in general there's no need to get me to review wpt metadata changes unless your primary reviewer doesn't feel comfortable reviewing those changes.
Attachment #8659353 - Flags: review?(james) → review+
(Assignee)

Comment 33

2 years ago
Created attachment 8659385 [details] [diff] [review]
part 4 - fix test_canvas.html tests with negative rects to no longer expect failure

This fixes the 2d.drawImage.negativedest and 2d.drawImage.negativesource tests to no longer be todo and expect to pass.

The case in 2d.drawImage.outsidesource that checked for negative width and height throwing an error needs to be removed since it should now actually pass, which causes the test overall to now pass, so the todo() call had to be fixed as well.
Attachment #8659385 - Flags: review?(jmuizelaar)
Attachment #8659385 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

2 years ago
Blocks: 655328
Comment on attachment 8658935 [details] [diff] [review]
part 1 - Normalize Canvas 2D rects to be positive by flipping negative width or height.

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2477,5 @@
>  //
>  // rects
>  //
>  
> +// bug 1074733

I'd rather have a spec citation than a bug number here.
Attachment #8658935 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 35

2 years ago
Created attachment 8659967 [details] [diff] [review]
part 1 - Normalize Canvas 2D rects to be positive by flipping negative width or height.

Cosmetic change only. Expanded comment to note that the canvas spec does not disallow negative dimensions in the rect, so we must generate an appropriate rect for the corner points.
Attachment #8658935 - Attachment is obsolete: true
Attachment #8659967 - Flags: review+
(Assignee)

Comment 36

2 years ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab765cedc709
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 37

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1007f0e9676
https://hg.mozilla.org/integration/mozilla-inbound/rev/00963cf3f9b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/88117f786e56
https://hg.mozilla.org/integration/mozilla-inbound/rev/51099b24b04a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b1007f0e9676
https://hg.mozilla.org/mozilla-central/rev/00963cf3f9b7
https://hg.mozilla.org/mozilla-central/rev/88117f786e56
https://hg.mozilla.org/mozilla-central/rev/51099b24b04a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.