1255576 - Resetting page zoom doesn't propagate across windows correctly

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and I've been seeing this for a bit; I just finally isolated when it was that resetting was failing...

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Propagate privacy status for removed content preferences to observers (Aurora patch)

This should fix it. Still need to write a test.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Recent regression, tracked for Fx47.

Comment 5

[Josh Matthews [:jdm]]

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?

Comment 6

[Drew Willcoxon :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

Comment 7

[Josh Matthews [:jdm]]

Attachment: Propagate privacy status for removed content preferences to observers

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8736415 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers

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

Thanks!

Comment 9

[Josh Matthews [:jdm]]

Attachment: Ensure IPC notifications for content preference observers include privacy status

Comment 10

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1539eb85b8b

Comment 11

[Josh Matthews [:jdm]]

Attachment: Propagate privacy status for removed content preferences to observers

Comment 12

[Josh Matthews [:jdm]]

Attachment: Ensure IPC notifications for content preference observers include privacy status

Comment 13

[Josh Matthews [:jdm]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0346ca3d9889
https://hg.mozilla.org/mozilla-central/rev/44371d79e863

Comment 15

[Josh Matthews [:jdm]]

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 16

[Josh Matthews [:jdm]]

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?

Comment 17

[Drew Willcoxon :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.

Comment 18

[Josh Matthews [:jdm]]

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

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Bz, could you please confirm that this issue is fixed as expected on Nightly? Thanks!

Comment 20

[Boris Zbarsky [:bzbarsky]]

Seems to be, yes.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Boris Zbarsky [:bz] from comment #20)
> Seems to be, yes.

Great! Thanks. Will approve uplift now.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729202 [details] [diff] [review]
Propagate privacy status for removed content preferences to observers (Aurora patch)

Fix was verified on Nightly, Aurora47+

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a9522d102bdf