Closed Bug 1574016 Opened Last month Closed Last month

patch from bug 1547277 contains logic error

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Regressed 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

bwerth noticed this. On this line

https://searchfox.org/mozilla-central/rev/3366c3d24f1c3818df37ec0818833bf085e41a53/layout/base/nsDocumentViewer.cpp#2100

we are missing a negation. This still fixed the bug because we seem to get another SetBounds call right after with the same bounds. But it does mean that we are probably invalidating when we don't need to.

The fixed still worked because we get another SetBounds call right after with the same bounds. But it does mean we are doing some useless invalidation.

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5cc355f51aa
Fix logic error in patch from bug 1547277. r=bradwerth
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Is this worth backporting to Beta? I'm thinking no for ESR if it's just causing wasted work but no correctness issues.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)

Is this worth backporting to Beta? I'm thinking no for ESR if it's just causing wasted work but no correctness issues.

I am in favor of backporting. Although we don't have a testcase for it, it seems possible that this would fail to invalidate a frame with differing bounds where the rootView has already been resized.

Comment on attachment 9085632 [details]
Bug 1574016. Fix logic error in patch from bug 1547277. r?bradwerth

Beta/Release Uplift Approval Request

  • User impact if declined: slightly more invalidating in some cases, possibly not painting a popup in a rare case
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): just invalidates in the correct case instead of accidentally getting the invalidation correct
  • String changes made/needed:
Flags: needinfo?(tnikkel)
Attachment #9085632 - Flags: approval-mozilla-beta?

Comment on attachment 9085632 [details]
Bug 1574016. Fix logic error in patch from bug 1547277. r?bradwerth

Fixes an invalidation bug, approved for 69.0b16.

Attachment #9085632 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1579732
You need to log in before you can comment on or make changes to this bug.