Progressive tiling code has a typo with possibly bad consequences

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: nical)

Tracking

unspecified
mozilla37
Other
Android
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox34 wontfix, firefox35 fixed, firefox36 fixed, firefox37 fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment)

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp?rev=aa785fd0b16a#219 is missing the <= 2 check

Saw this while reviewing the new contributor patch in bug 1113774. Don't have time at the moment to dig into the history or implications of this bug but i can do it tomorrow if nobody gets to it before me.
Posted patch Fix the typo.Splinter Review
Attachment #8540174 - Flags: review?(bugmail.mozilla)
Comment on attachment 8540174 [details] [diff] [review]
Fix the typo.

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

This might need uplifting, please check to see which branches this affects and uplift as needed.
Attachment #8540174 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → nical.bugzilla
The typo landed a year ago so pretty much all branches are affected. I'll let it bake on central for a day or two and see how things go. Not sure whether we want to uplift all the way to beta considering we haven't seen that it caused issues so far (or haven't made the connection).
It might be related to the things benwa is trying to fix in bug 1112332.
Comment on attachment 8540174 [details] [diff] [review]
Fix the typo.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: We can get into incorrect situations with low precision tiling, currently defaults on B2G and Mac.
[Describe test coverage new/current, TBPL]:
[Risks and why]: Low risk.
[String/UUID change made/needed]:
Attachment #8540174 - Flags: approval-mozilla-beta?
Attachment #8540174 - Flags: approval-mozilla-b2g34?
Attachment #8540174 - Flags: approval-mozilla-aurora?
[Blocking Requested - why for this release]:
We heavily use tiling, including low precision on B2G, every time we scroll.  This problem can lead to noticeable but hard to diagnose correctness problems.
blocking-b2g: --- → 2.1?
blocking-b2g: 2.1? → 2.1+
https://hg.mozilla.org/mozilla-central/rev/0076eb0ed38c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8540174 - Flags: approval-mozilla-beta?
Attachment #8540174 - Flags: approval-mozilla-beta+
Attachment #8540174 - Flags: approval-mozilla-aurora?
Attachment #8540174 - Flags: approval-mozilla-aurora+
Is this bad enough to warrant 2.0 blocking status (and a b2g32 approval request)?
Flags: needinfo?(nical.bugzilla)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #10)
> Is this bad enough to warrant 2.0 blocking status (and a b2g32 approval
> request)?

I don't know how bad things need to be to warrant 2.0 blocking status, so I don't think I am the best person to answer this question. The fix is rather simple and low-risk so I would argue if favour of taking it to 2.0. forwarding the needinfo to Kats.
Flags: needinfo?(nical.bugzilla) → needinfo?(bugmail.mozilla)
I would say no. I don't know how many people are running 2.0 within Mozilla anymore and if this patch were to cause regressions it's unlikely it would be noticed before it shipped to users. Based on the testing so far we don't have any concrete evidence that this typo actually manifests as something bad so I would say to leave it out of 2.0.
Flags: needinfo?(bugmail.mozilla)
Attachment #8540174 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
You need to log in before you can comment on or make changes to this bug.