Closed Bug 1255576 Opened 4 years ago Closed 4 years ago

Resetting page zoom doesn't propagate across windows correctly

Categories

(Firefox :: General, defect)

47 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 --- unaffected
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: jdm)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

STEPS TO REPRODUCE:

1)  Start current nightly (in e10s mode, but it doesn't seem to actually
    matter).
2)  Open two windows.
3)  Load http://mxr.mozilla.org/ in both windows.
4)  Focus one of the windows.
5)  Zoom in (Cmd-+ on Mac) 4 times.
6)  Reset the zoom by selecting Cmd-0 or the "Reset" option from View > Zoom.

EXPECTED RESULTS: After step 5 both windows show large text.  After step 6 both windows go back to normal.

ACTUAL RESULTS: After step 6, the window that was not the focused one does NOT go back to normal.

ADDITIONAL INFORMATION:

A)  This works correctly for multiple tabs in the same window.  It just doesn't work correctly across windows.

B)  I tried doing this with non-e10s windows and the problem seems to be present anyway.

I do see code in browser/base/content/browser-fullZoom.js that tries to deal with this (e.g. an onContentPrefRemoved implementation).  I'm not sure why it's not working correctly.
Oh, and I've been seeing this for a bit; I just finally isolated when it was that resetting was failing...
Nightly regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=941033a51983ddec2d99aa9f868a54c0196a4075&tochange=5f9ba76eb3b1fd9377bbdb4cc2f98a7e75eabdfb

Skimming that, looks like a regression from bug 1089232: the aIsPrivate argument was added to onContentPrefSet but not to onContentPrefRemoved.  So now in the removed case we're calling _onContentPrefChanged with aIsPrivate === undefined, and since both false == undefined and true == undefined are false we never reach this._applyPrefToZoom in there.
Blocks: 1089232
Flags: needinfo?(josh)
Flags: needinfo?(adw)
Keywords: regression
Assignee: nobody → josh
Flags: needinfo?(josh)
Flags: needinfo?(adw)
This should fix it. Still need to write a test.
Recent regression, tracked for Fx47.
Drew, while writing tests for this I discovered that ContentPrefService2._removeByDomain behaves oddly. In particular, if the provided context is private the code ends up clearing out the private browsing storage, without doing any kind of domain comparison. Is this intentional?
Flags: needinfo?(adw)
Oh wow, no, I don't think that's supposed to be the behavior, thanks for asking.  The test misses that case, too.  I'm sure you found it, but it's here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/test_removeByDomain.js#124
Flags: needinfo?(adw)
Attachment #8729202 - Attachment is obsolete: true
Comment on attachment 8736415 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers

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

Thanks!
Attachment #8736415 - Flags: review?(adw) → review+
Attachment #8736434 - Flags: review?(adw) → review+
Attachment #8736415 - Attachment is obsolete: true
Attachment #8736434 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0346ca3d9889f94d41e2a86e80c3b08c97ac951b
Bug 1255576 - Propagate privacy status for removed content preferences to observers. r=adw

https://hg.mozilla.org/integration/mozilla-inbound/rev/44371d79e8631b542556c52a7cc015414b487eca
Bug 1255576 - Ensure IPC notifications for content preference observers include privacy status. r=adw
https://hg.mozilla.org/mozilla-central/rev/0346ca3d9889
https://hg.mozilla.org/mozilla-central/rev/44371d79e863
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I'm investigating if there's a smaller, more targeted fix that's possible for uplifting to 47. 36kb of patch makes me uneasy.
Comment on attachment 8729202 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers (Aurora patch)

This patch fixes the original regression reported by bz. I am inclined to uplift this patch rather than the full patches that landed on m-c to avoid the risk of additional regressions. Any opinion, Drew?
Attachment #8729202 - Attachment description: Propagate privacy status for removed content preferences to observers → Propagate privacy status for removed content preferences to observers (Aurora patch)
Attachment #8729202 - Attachment is obsolete: false
Attachment #8729202 - Flags: feedback?(adw)
Comment on attachment 8729202 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers (Aurora patch)

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

Sounds good to me, thanks Josh.
Attachment #8729202 - Flags: feedback?(adw) → feedback+
Comment on attachment 8729202 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers (Aurora patch)

Approval Request Comment
[Feature/regressing bug #]: 1089232
[User impact if declined]: Possibility of unexpected UI interactions when visiting the same site in multiple tabs/windows.
[Describe test coverage new/current, TreeHerder]: Tests for original patch on mozilla-central.
[Risks and why]: Should be risk-proof, as new argument is optional and defaults to a sensible value. 
[String/UUID change made/needed]: None
Attachment #8729202 - Flags: approval-mozilla-aurora?
Bz, could you please confirm that this issue is fixed as expected on Nightly? Thanks!
Flags: needinfo?(bzbarsky)
Seems to be, yes.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #20)
> Seems to be, yes.

Great! Thanks. Will approve uplift now.
Comment on attachment 8729202 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers (Aurora patch)

Fix was verified on Nightly, Aurora47+
Attachment #8729202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Version: unspecified → 47 Branch
You need to log in before you can comment on or make changes to this bug.