Closed Bug 555390 Opened 14 years ago Closed 9 years ago

Intermittent failure in browser_bug419612.js | Zoom should affect background tabs - Got 1, expected 1.100000023841858

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86
Windows Server 2003
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME
Milestone:
Firefox 27
Tracking Flags:
Tracking Status
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed
firefox-esr24 --- fixed

People

(Reporter: philor, Assigned: adw)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

wait for location-change after tab removal
12.46 KB, patch
mak
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug 555120 +++

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269645944.1269647449.32611.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/03/26 16:25:44
s: win32-slave27

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug419612.js | Zoom should affect background tabs - Got 1, expected 1.100000023841858
Assignee: nobody → rflint
Status: NEW → ASSIGNED
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269907989.1269909719.5405.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/03/29 17:13:09
s: win32-slave34
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269977548.1269979204.14426.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/03/30 12:32:28
s: win32-slave28
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269987174.1269988788.12091.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/03/30 15:12:54
s: win32-slave32
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270038083.1270039700.787.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/03/31 05:21:23
s: win32-slave27
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270151745.1270153771.6189.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/04/01 12:55:45
s: win32-slave33
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270165121.1270166804.9927.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/04/01 16:38:41
s: win32-slave32
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272345732.1272347344.22980.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/04/26 22:22:12  
s: win32-slave16
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug419612.js | Zoom should affect background tabs - Got 1, expected 1.100000023841858
Sort of vaguely interesting that this apparently got fixed by something, but bug 555120 continues on.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Whiteboard: [orange]
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Drew, is this something you could look at given your recent work on similar bugs?
Assignee: rflint → nobody
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
Flags: needinfo?(adw)
Yes, sorry to keep bothering you guys with these zoom-related oranges. :-\
Assignee: nobody → adw
Status: REOPENED → ASSIGNED
Flags: needinfo?(adw)
The failing test looks fine, so I think this is another case where previous tests are interfering with the failing test because they're not waiting for the location-change notification.

browser_bug416661.js runs two tests ahead of the failing test.  The last thing it does is remove a tab.  Removing a tab causes another tab to become selected, and when a tab is selected, the location-change notification is broadcasted.  I think what's happening is that the notification broadcasted due to this tab removal is sometimes caught by the failing test, which should not happen.

This patch adds a function to FullZoomHelper that removes a tab and waits for the location-change notification before continuing, and it updates all necessary zoom tests to use it.

https://tbpl.mozilla.org/?tree=Try&rev=fefa6994a907
Comment on attachment 810326 [details] [diff] [review]
wait for location-change after tab removal

The try results look good, but this orange is pretty infrequent so who knows.  Comment 53 explains this patch, and even though it's test-only I'm asking for review since it's not trivial.

This patch also removes a few lines from browser-fullZoom.js that I accidentally duplicated in an earlier bug.
Attachment #810326 - Flags: review?(mak77)
Comment on attachment 810326 [details] [diff] [review]
wait for location-change after tab removal

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

::: browser/base/content/test/general/head.js
@@ +297,5 @@
>      if (tab && gBrowser.selectedTab == tab) {
>        deferred.resolve();
>        return deferred.promise;
>      }
>      if (tab)

instead of null checking every time tab, looks like you may just throw at the beginning for a null tab, since I see that now you are directly using waitForLocationChange instead of selectTabAndWaitForLocationChange(null)

@@ +300,5 @@
>      }
>      if (tab)
>        gBrowser.selectedTab = tab;
> +    this.waitForLocationChange().then(() => deferred.resolve());
> +    return deferred.promise;

why don't you directly return this.waitForLocationChange()?

@@ +312,5 @@
> +    if (selected)
> +      this.waitForLocationChange().then(() => deferred.resolve());
> +    else
> +      deferred.resolve();
> +    return deferred.promise;

if (selected) return this.waitFor();
else return Promise.resolve();
Attachment #810326 - Flags: review?(mak77) → review+
ehr, the last comment without the else!
https://hg.mozilla.org/mozilla-central/rev/fa17cd3d7f34
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Inactive; closing (see bug 1180138).
Status: REOPENED → RESOLVED
Closed: 11 years ago → 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: