Intermittent browser/base/content/test/tabs/browser_preloadedBrowser_zoom.js | zoomed in applied to preloaded: 1.1 - Got 1, expected 1.1

RESOLVED FIXED in Firefox 61

Status

()

defect
P5
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: intermittent-bug-filer, Assigned: arai)

Tracking

({intermittent-failure})

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [stockwell fixed:timing])

Attachments

(2 attachments, 1 obsolete attachment)

We have 64 failures in the last 7 days.
They occur mostly on Windows 7 (opt, pgo) and the rest of them on windows10-64 (pgo), windows7-32-devedition (opt), windows7-32-nightly (opt).
recent failure log example: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=169184234&lineNumber=4457
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1397098&startday=2018-03-13&endday=2018-03-20&tree=all
Flags: needinfo?(dao+bmo)
Whiteboard: [stockwell needswork]
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Posted patch bug1397098.patch (obsolete) — Splinter Review
Disable on Windows 7 non-debug builds (opt, pgo).
Attachment #8961677 - Flags: review?(jmaher)
Comment on attachment 8961677 [details] [diff] [review]
bug1397098.patch

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

::: browser/base/content/test/tabs/browser.ini
@@ +28,4 @@
>  [browser_pinnedTabs_closeByKeyboard.js]
>  [browser_positional_attributes.js]
>  [browser_preloadedBrowser_zoom.js]
> +skip-if = !debug || (os == 'win' && (os_version == '6.1')) # Bug 1397098

this || logic will skip the test on all opt/pgo OR windows 7 opt/pgo/debug.
Attachment #8961677 - Flags: review?(jmaher) → review-
I've replaced || with && and created a new patch.
Attachment #8961677 - Attachment is obsolete: true
Attachment #8961690 - Flags: review?(jmaher)
Attachment #8961690 - Flags: review?(jmaher) → review+
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e2027885e1
disable browser/base/content/test/tabs/browser_preloadedBrowser_zoom.js for frequent failures. r=jmaher
Keywords: checkin-needed
_preloadedBrowser's fullZoom is changed from 1 to 1.1 in the following place:

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/toolkit/content/widgets/remote-browser.xml#164-176
>   <binding id="remote-browser" extends="chrome://global/content/bindings/browser.xml#browser">
> ...
>       <field name="_fullZoom">1</field>
>       <property name="fullZoom">
> ...
>         <setter><![CDATA[
>           let changed = val.toFixed(2) != this._fullZoom.toFixed(2);
> 
>           this._fullZoom = val;
>           try {
>             this.messageManager.sendAsyncMessage("FullZoom", {value: val});
>           } catch (ex) {}
> 
>           if (changed) {
>             let event = new Event("FullZoomChange", {bubbles: true});
>             this.dispatchEvent(event);
>           }
>         ]]></setter>

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/toolkit/content/viewZoomOverlay.js#57
> var ZoomManager = {
> ...
>   setZoomForBrowser: function ZoomManager_setZoomForBrowser(aBrowser, aVal) {
> ...
>     if (this.useFullZoom || aBrowser.isSyntheticDocument) {
>       aBrowser.textZoom = 1;
>       aBrowser.fullZoom = aVal;

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/browser/base/content/browser-fullZoom.js#348
> var FullZoom = {
> ...
>   _applyPrefToZoom: function FullZoom__applyPrefToZoom(aValue, aBrowser, aCallback) {
> ...
>     if (aValue !== undefined) {
>       ZoomManager.setZoomForBrowser(aBrowser, this._ensureValid(aValue));

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/browser/base/content/browser-fullZoom.js#246-247
> var FullZoom = {
> ...
>   onLocationChange: function FullZoom_onLocationChange(aURI, aIsTabSwitch, aBrowser) {
> ...
>     this._cps2.getByDomainAndName(aURI.spec, this.name, ctxt, {
> ...
>       handleCompletion: () => {
> ...
>         this._applyPrefToZoom(value, browser,
>                               this._notifyOnLocationChange.bind(this, browser));


https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/toolkit/components/contentprefs/ContentPrefUtils.jsm#46
> function safeCallback(callbackObj, methodName, args) {
> ...
>   try {
>     callbackObj[methodName].apply(callbackObj, args);

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/toolkit/components/contentprefs/ContentPrefUtils.jsm#35
> function cbHandleCompletion(callback, reason) {
>   safeCallback(callback, "handleCompletion", [reason]);

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/toolkit/components/contentprefs/ContentPrefService2.js#192
> ContentPrefService2.prototype = {
> ...
>   _get: function CPS2__get(group, name, includeSubdomains, context, callback) {
> ...
>     this._execStmts([this._commonGetStmt(group, name, includeSubdomains)], {
> ...
>       onDone: function onDone(reason, ok, gotRow) {
> ...
>         cbHandleCompletion(callback, reason);

https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/toolkit/components/contentprefs/ContentPrefService2.js#785-788
> ContentPrefService2.prototype = {
> ...
>   _execStmts: function CPS2__execStmts(stmts, callbacks) {
>     this._dbConnection.executeAsync(stmts, stmts.length, {
> ...
>       handleCompletion: function handleCompletion(reason) {
>         try {
>           let ok = reason == Ci.mozIStorageStatementCallback.REASON_FINISHED;
>           callbacks.onDone.call(self,
>                                 ok ? Ci.nsIContentPrefCallback2.COMPLETE_OK :
>                                   Ci.nsIContentPrefCallback2.COMPLETE_ERROR,
>                                 ok, gotRow);

So, the possibility is, the value is somehow set to 1 (maybe the zoom-in is not triggered twice), or not yet changed to 1.1 at the point of checking the value.

what bug 1442465 changed is, not to wait for SessionStore update after removing tab, so, it might be that the removed tab's information is not yet reflected to _preloadedBrowser.

anyway, I cannot reproduce the issue on win10 x64.
I'll check on try with extra logging
(currently I don't have enough disk space to setup win7 env :P
apparently it's set from 1 to 1.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=301bfc8bd544bf29dac6b17527c5499e3a2666e6&selectedJob=170023490
I'll check the source of the "1" there and when it's supposed to be changed to 1.1
So, the value is set here
https://searchfox.org/mozilla-central/source/browser/base/content/browser-fullZoom.js#376-378
>   _applyZoomToPref: function FullZoom__applyZoomToPref(browser) {
> ...
>     this._cps2.set(browser.currentURI.spec, this.name,
>                    ZoomManager.getZoomForBrowser(browser),
>                    this._loadContextFromBrowser(browser), {

this is called twice from the test, synchronously, but I think the update itself is async.
maybe we should make sure the update finishes.
(In reply to Tooru Fujisawa [:arai] from comment #33)
> here's try
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=aedd9edcd963050049efbbef8537db91649de8fb&filter-
> searchStr=win%20bc%20opt

forgot to enable the test :P

https://treeherder.mozilla.org/#/jobs?repo=try&revision=918e26b9f828b3a3b317c6530de2b2cbbd04ed80
So, looks like the update of the zoom level content pref happens asynchronously after changing the zoom level, and the default zoom level of the preloaded browser depends on the value.
So, we should wait for the update directly (as long as there's no better indirect way), after changing zoom level, before creating preloaded browser.
Flags: needinfo?(arai.unmht)
Attachment #8962010 - Flags: review?(adw)
Attachment #8962010 - Flags: review?(adw) → review+
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:timing]
You need to log in before you can comment on or make changes to this bug.