Intermittent test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: not zoomed out, got 1

RESOLVED FIXED in Firefox 28

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: RyanVM, Assigned: adw)

Tracking

({intermittent-failure})

Trunk
mozilla29
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 wontfix, firefox28 fixed, firefox29 fixed, firefox-esr24 wontfix)

Details

Attachments

(1 attachment)

https://tbpl.mozilla.org/php/getParsedLog.php?id=21302365&tree=Mozilla-Inbound

Rev5 MacOSX Mountain Lion 10.8 mozilla-inbound opt test mochitest-1 on 2013-03-31 17:45:06 PDT for push 4222a898dbae
slave: talos-mtnlion-r5-051

17:50:52     INFO -  101759 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Shouldn't zoom by vertical/positive pixel event when its lineOrPageDeltaY is 0: scrolled vertical
17:50:52     INFO -  101760 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Shouldn't zoom by vertical/positive pixel event when its lineOrPageDeltaY is 0: scrolled horizontal
17:50:52     INFO -  101761 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Shouldn't zoom by vertical/positive pixel event when its lineOrPageDeltaY is 0: zoomed
17:50:52     INFO -  101762 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: scrolled vertical
17:50:52     INFO -  101763 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: scrolled horizontal
17:50:52     INFO -  101764 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: not zoomed out, got 1
Drew, I don't suppose this also falls into the "zoom bugs Drew's looking into" category, does it? :)
Flags: needinfo?(adw)
Sure.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
I think what's happening is: The test resets the zoom, which changes the visual zoom immediately and removes the page's zoom pref from content prefs.  A little while later FullZoom is notified that a pref has been removed -- the same pref it just removed -- so it applies the default zoom to the page.  But by that point the test has scrolled the mouse wheel, changing the zoom.  So you end up with this sequence: test resets zoom, test scrolls mouse wheel, FullZoom sees the pref removal and effectively resets the zoom again.

There was a similar intermittent failure a while ago, and the fix there was to stop FullZoom from updating the visual zoom when it saw an update for a pref whose visual zoom had already been applied.  I think that may be the right behavior in all cases.

https://tbpl.mozilla.org/?tree=Try&rev=7853b8e5a28e
(In reply to Drew Willcoxon :adw from comment #140)
> I think what's happening is: The test resets the zoom, which changes the
> visual zoom immediately and removes the page's zoom pref from content prefs.
> A little while later FullZoom is notified that a pref has been removed --
> the same pref it just removed -- so it applies the default zoom to the page.
> But by that point the test has scrolled the mouse wheel, changing the zoom. 
> So you end up with this sequence: test resets zoom, test scrolls mouse
> wheel, FullZoom sees the pref removal and effectively resets the zoom again.
> 
> There was a similar intermittent failure a while ago, and the fix there was
> to stop FullZoom from updating the visual zoom when it saw an update for a
> pref whose visual zoom had already been applied.  I think that may be the
> right behavior in all cases.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=7853b8e5a28e

Any more luck with this? :-)
Bug 555120 fixes this one, too.  Waiting on review.
(In reply to Drew Willcoxon :adw from comment #181)
> Bug 555120 fixes this one, too.  Waiting on review.

Great! Thank you for the update :-)
Depends on: 555120
Target Milestone: --- → mozilla24
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch patchSplinter Review
I think the test just needs to wait for the actual zoom change after it synthesizes accel-0 to reset the zoom.  This works locally, but I'll try it many times on try.

https://tbpl.mozilla.org/?tree=Try&rev=034482b952d3
Comment on attachment 8366378 [details] [diff] [review]
patch

Try looks OK.

Mike, you've worked on browser-fullZoom.js a little, so would you mind reviewing this change?  The reason for using _executeSoon when firing the notification is that _getGlobalValue is sometimes async and sometimes sync.  Using _executeSoon makes the notification behavior consistent from observers' points of view and allows them to always do this:

  resetZoom();
  onZoomChange(...); // i.e., addObserver(function () {}, topic, false);

Otherwise they'd have to add the observer first, which is more awkward IMO.
Attachment #8366378 - Flags: review?(mdeboer)
Whiteboard: [triage]
Comment on attachment 8366378 [details] [diff] [review]
patch

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

Clearing review atm because:

(In reply to Drew Willcoxon :adw from comment #229)
> The reason for using _executeSoon when firing the
> notification is that _getGlobalValue is sometimes async and sometimes sync.

It seems to me that ensuring _getGlobalValue to be _always_ async with an `_executeSoon()` would be a better improvement than handling this case only, right?
No API consumer/ dev will expect that a continuation-passing-style function can be sync, so I think it'd be good to fix the core issue here.
Attachment #8366378 - Flags: review?(mdeboer)
Comment on attachment 8366378 [details] [diff] [review]
patch

The _getGlobalValue comment explains why it works like that.  Please read it, but the point is that we want to keep paths sync whenever possible to have the best possible zooming UX.  This behavior is by design, internal to FullZoom, and was already r+'ed when it was originally reviewed.  So I'm requesting review again on the same patch.
Attachment #8366378 - Flags: review?(mdeboer)
Comment on attachment 8366378 [details] [diff] [review]
patch

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

Hi Drew, thanks for explaining the story behind sync/async `_getGlobalValue`!

With that in mind, I think this makes perfect sense.

(I meant to post this yesterday, but somehow I forgot to push the button :/)

::: browser/base/content/browser-fullZoom.js
@@ +329,5 @@
>      this._getGlobalValue(browser.contentWindow, function (value) {
>        if (token.isCurrent) {
>          ZoomManager.setZoomForBrowser(browser, value === undefined ? 1 : value);
>          this._ignorePendingZoomAccesses(browser);
> +        this._executeSoon(function () {

please add a comment explaining why this is here for posterity
Attachment #8366378 - Flags: review?(mdeboer) → review+
Thanks, landed with comment addressed.

https://hg.mozilla.org/integration/fx-team/rev/d21d87af5436
https://hg.mozilla.org/mozilla-central/rev/d21d87af5436
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Drew, can you please request Aurora approval for this?
Flags: needinfo?(adw)
Target Milestone: mozilla24 → mozilla29
Comment on attachment 8366378 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): intermittent orange
User impact if declined: none, test infrastructure will benefit though
Testing completed (on m-c, etc.): 1 day on m-c
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a.
Attachment #8366378 - Flags: approval-mozilla-aurora?
Flags: needinfo?(adw)
Comment on attachment 8366378 [details] [diff] [review]
patch

Thanks Mike. Approved.
Attachment #8366378 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: fxdesktopbacklog
Whiteboard: [triage]
You need to log in before you can comment on or make changes to this bug.