Last Comment Bug 749467 - canvas renders incorrectly
: canvas renders incorrectly
Status: VERIFIED FIXED
: regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: 7 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on: 749975
Blocks: 651858
  Show dependency treegraph
 
Reported: 2012-04-26 18:01 PDT by ashvinjayaram
Modified: 2012-05-30 19:15 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
affected
+
verified
+
verified


Attachments
Properly mark mPathTransformWillUpdate false (814 bytes, patch)
2012-04-26 21:12 PDT, Bas Schouten (:bas.schouten)
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Add reftest for path continuation after transform changes (2.10 KB, patch)
2012-04-26 21:13 PDT, Bas Schouten (:bas.schouten)
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description ashvinjayaram 2012-04-26 18:01:10 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.162 Safari/535.19

Steps to reproduce:

Reduced the issue I was having to this test case: http://jsfiddle.net/una3y/


Actual results:

Firefox 12 renders this differently to all other browsers, including older versions of Firefox
Comment 1 Alice0775 White 2012-04-26 19:39:19 PDT
Confirmed on 
http://hg.mozilla.org/mozilla-central/rev/cc5254f9825f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120426 Firefox/15.0a1 ID:20120426030504

And gfx.canvas.azure.enabled = false helps.


Regression window(m-c):
Works:
http://hg.mozilla.org/mozilla-central/rev/450e4d9ea2d5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 ID:20110625025936
Fails:
http://hg.mozilla.org/mozilla-central/rev/48ad4ffc4230
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110625 Firefox/7.0a1 ID:20110625030821
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=450e4d9ea2d5&tochange=48ad4ffc4230



Regression window(m-i):
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c65f1fb0449d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 ID:20110624094307
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/46c3d095bc75
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 ID:20110624104626
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c65f1fb0449d&tochange=46c3d095bc75


Regressed by:
Bug 651858
Comment 2 ashvinjayaram 2012-04-26 19:47:02 PDT
Adding an extra ctx.stroke() fixes this for now: http://jsfiddle.net/una3y/1/
Comment 3 Bas Schouten (:bas.schouten) 2012-04-26 21:12:38 PDT
Created attachment 618920 [details] [diff] [review]
Properly mark mPathTransformWillUpdate false

So the problem here is that ::EnsureWritablePath will create a device space path builder. But it will never mark mPathTransformWillUpdate false to indicate the changing transform (caused by the restore) has been dealt with. This patch fixes that, reftest incoming.
Comment 4 Bas Schouten (:bas.schouten) 2012-04-26 21:13:09 PDT
Created attachment 618921 [details] [diff] [review]
Add reftest for path continuation after transform changes
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-26 21:35:01 PDT
Need to get this regression fix everywhere.
Comment 8 Alex Keybl [:akeybl] 2012-04-30 15:10:04 PDT
It's not yet clear whether this would be a ride along for a FF12 chemspill - we'll evaluate at that time. We should, however, get this fix on Aurora and Beta already.
Comment 9 Alex Keybl [:akeybl] 2012-05-02 15:30:07 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Need to get this regression fix everywhere.

Actually, can we have more insight into why you feel this regression from FF7 needs to land on all branches?
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-02 17:04:28 PDT
Because Web compatibility breakage regressions are really bad?
Comment 11 Alex Keybl [:akeybl] 2012-05-14 09:23:03 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Because Web compatibility breakage regressions are really bad?

Yes, they're bad. But "Need to get this regression fix everywhere" implies urgency undue to a regression this old. We'll evaluate risk/reward when the fix is ready.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-14 15:08:17 PDT
The fix has already landed on mozilla-central. What else are we waiting for?
Comment 13 Bas Schouten (:bas.schouten) 2012-05-14 15:18:26 PDT
Comment on attachment 618920 [details] [diff] [review]
Properly mark mPathTransformWillUpdate false

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Comment 14 Bas Schouten (:bas.schouten) 2012-05-14 15:20:09 PDT
(In reply to Bas Schouten (:bas) from comment #13)
> Comment on attachment 618920 [details] [diff] [review]
> Properly mark mPathTransformWillUpdate false
> 
> [Approval Request Comment]
> Regression caused by (bug #):
Bug 651858 
> User impact if declined:
Canvas rendering incorrect
> Testing completed (on m-c, etc.):
Landed on m-c
> Risk to taking this patch (and alternatives if risky): 
Low risk - I don't see any serious risks.
> String changes made by this patch:
None
Comment 15 Bas Schouten (:bas.schouten) 2012-05-14 15:20:46 PDT
Comment on attachment 618921 [details] [diff] [review]
Add reftest for path continuation after transform changes

If we uplift the patch we might want to uplift the test as well, not sure what the policy is there.
Comment 16 Alex Keybl [:akeybl] 2012-05-14 15:28:14 PDT
Comment on attachment 618920 [details] [diff] [review]
Properly mark mPathTransformWillUpdate false

[Triage Comment]
Given the risk evaluation, approving the fix for this regression for both Aurora 14 and Beta 13. If we see any regressions over the next 3 weeks, we'll be quick to back this out (although that's unexpected).
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-14 15:32:56 PDT
I will land this.
Comment 19 Ioana (away) 2012-05-30 05:54:54 PDT
The automated test for this bug passed on all OSs on Firefox 13.0 beta:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12159689&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12163243&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12164769&full=1&branch=mozilla-beta

I have also tried to verify this fix manually but I got the same image rendered on:
IE9
Chrome 19.0.1084.46
Firefox 11.0
Firefox 12.0
Firefox 13.0 beta 6.

If anyone can reproduce this issue on pre-fix builds, please try to verify this issue manually.
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-30 16:29:57 PDT
Alice, would you mind confirming if this is fixed for you? It seems we have inconsistent results as per comment 19.
Comment 21 Alice0775 White 2012-05-30 18:19:42 PDT
I can reproduce the Issue in
http://hg.mozilla.org/releases/mozilla-release/rev/a294a5b4f12d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0 ID:20120420145725

I cannot Reproduce anymore in
http://hg.mozilla.org/releases/mozilla-beta/rev/5de6e4bd8ede
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0 ID:20120523114940
http://hg.mozilla.org/releases/mozilla-aurora/rev/89176b6d643c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120530 Firefox/14.0a2 ID:20120530042008
http://hg.mozilla.org/mozilla-central/rev/65fa5cb6f79c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120530030519
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-30 19:15:33 PDT
Thanks a lot, Alice!

Note You need to log in before you can comment on or make changes to this bug.