Closed Bug 1775671 Opened 3 years ago Closed 10 months ago

Intermittent browser/components/customizableui/test/browser_947914_button_zoomReset.js | single tracking bug

Categories

(Firefox :: Toolbars and Customization, defect, P3)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
143 Branch
Tracking Flags:
Tracking Status
firefox143 --- fixed

People

(Reporter: jmaher, Assigned: emilio)

References

Details

(Keywords: intermittent-failure, intermittent-testcase)

Attachments

(1 file, 1 obsolete file)

Bug 1775671 - Prevent a race in browser_947914_button_zoomReset.js. r=Gijs,florian
48 bytes, text/x-phabricator-request
Details | Review
No description provided.

Additional information about this bug failures and frequency patterns can be found by running: ./mach test-info failure-report --bug 1775671

Severity: -- → N/A
Attachment #9384197 - Attachment is obsolete: true

https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
For more information, please visit BugBot documentation.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE

New failure instance: https://treeherder.mozilla.org/logviewer?job_id=451277901&repo=autoland

Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---

There have been 66 total failures in the last 7 days.
There are:

  • 18 failures on linux1804-64-tsan-qr opt
  • 18 failures on linux2404-64 debug
  • 4 failures on macosx1015-64-qr debug
  • 6 failures on windows11-32-24h2 opt and debug
  • 20 failures on windows11-64-24h2 opt and debug

Recent failure log.

Gijs, as the owner of this component, can you help us assign the bug to someone? Thank you.

Flags: needinfo?(gijskruitbosch+bugs)

Looks like this started being sad more often roughly with the push from https://hg-edge.mozilla.org/mozilla-central/pushloghtml?changeset=27f05f28bba1befdade8fbfde9de8e51be444a6a

I wonder if changing the zoom for the page loaded with withNewTab needs to wait for something else here, or something?

Emilio, you previously fixed this test in bug 1612427 - don't suppose you have ideas here? Also, Florian, obviously bug 1702637 stands out in there, any ideas?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
Flags: needinfo?(emilio)

I can reproduce this fairly easily on a debug build. When it fails, the page zoom is 100% (rather than the expected 0.5).

I don't see what in ZoomManager.zoom = 0.5 would invalidate the token here which would set it to 100%.

Something like this seems to make the test reliable locally:

diff --git a/browser/components/customizableui/test/browser_947914_button_zoomReset.js b/browser/components/customizableui/test/browser_947914_button_zoomReset.js
index c97e2f17d15c0..52de9d29ddbdb 100644
--- a/browser/components/customizableui/test/browser_947914_button_zoomReset.js
+++ b/browser/components/customizableui/test/browser_947914_button_zoomReset.js
@@ -32,7 +32,7 @@ add_task(async function () {
           gBrowser,
           "FullZoomChange"
         );
-        ZoomManager.zoom = 0.5;
+        FullZoom.setZoom(0.5, gBrowser.selectedBrowser);
         await zoomChange;
       }

But the question now of course is "should ZoomManager.zoom go via FullZoom instead, or something else?

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #132)

I can reproduce this fairly easily on a debug build. When it fails, the page zoom is 100% (rather than the expected 0.5).

I don't see what in ZoomManager.zoom = 0.5 would invalidate the token here which would set it to 100%.

Is this a case where adding profiler markers with stacks would help figure out what's happening?

Flags: needinfo?(florian)

(In reply to Florian Quèze [:florian] from comment #133)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #132)

I can reproduce this fairly easily on a debug build. When it fails, the page zoom is 100% (rather than the expected 0.5).

I don't see what in ZoomManager.zoom = 0.5 would invalidate the token here which would set it to 100%.

Is this a case where adding profiler markers with stacks would help figure out what's happening?

The way I read Emilio's comment is that it's expected that this can fail given the ordering right now. Though tbf then in my head (and the reason I hadn't responded yet) the next question would be "so why does it work the rest of the time?". It's possible markers would help with that.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #132)

But the question now of course is "should ZoomManager.zoom go via FullZoom instead, or something else?

I think naively that on its own won't work because ZoomManager is toolkit code and FullZoom is browser code.

Unfortunately I don't have time to dive into this in the near future. :-(

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
Flags: needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #134)

The way I read Emilio's comment is that it's expected that this can fail given the ordering right now.

Correct

Though tbf then in my head (and the reason I hadn't responded yet) the next question would be "so why does it work the rest of the time?". It's possible markers would help with that.

So what's going on is:

  1. The tab is opened and the page loads.
  2. The FullZoom code requests the site-specific zoom value from the content prefs. This is async.
  3. The value arrives, and the tab gets a full zoom of 1 (100%).
  4. The tests sets ZoomManager.zoom = 0.5

If 3 happens before 4, the test works. If 4 happens before 3, the test fails (because the zoom is 1 rather than 0.5).

I think naively that on its own won't work because ZoomManager is toolkit code and FullZoom is browser code.

I guess we could make ZoomManager.zoom fire some kind of event which would invalidate the FullZoom content prefs request... But I'm not the most familiar with that code and I don't have lots of spare cycles. Something like the diff in comment 132 should be harmless and fix this (along with a follow-up on file to deal with this more systematically).

Flags: needinfo?(emilio)

So what's going on is:

  1. The tab is opened and the page loads.
  2. The FullZoom code requests the site-specific zoom value from the content prefs. This is async.
  3. The value arrives, and the tab gets a full zoom of 1 (100%).
  4. The tests sets ZoomManager.zoom = 0.5

If 3 happens before 4, the test works. If 4 happens before 3, the test
fails (because the zoom is 1 rather than 0.5).

Using FullZoom directly helps, since that invalidates the content prefs
lookup effectively, if it is still in flight.

Assignee: nobody → emilio
Whiteboard: [stockwell disable-recommended]
No longer depends on: 1685314

https://hg.mozilla.org/mozilla-central/rev/3c046e5371bd

Status: REOPENED → RESOLVED
Closed: 2 years ago → 10 months ago
status-firefox143: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 143 Branch
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: