Closed Bug 1397098 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P5)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

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

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:timing])

Attachments

(2 files, 1 obsolete file)

bug1397098.patch
879 bytes, patch
jmaher
: review+
Details | Diff | Splinter Review
Wait for the content pref update after changing zoom level.
2.80 KB, patch
adw
: review+
Details | Diff | Splinter Review 
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)
Attached 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-
Attached patch bug1397098.patch — — Splinter 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aca95a7383ae0f4c34b38dee0827da7de81eb95
Bug 1397098 - Wait for the content pref update after changing zoom level. r=adw
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: