1546075 - Thunderbird 2019-04-20 trunk zoom in/out don't work

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Kamens]

In the nightly build of 2019-04-20, neither the zoom menu commands nor key bindings works. Tested in safe mode, same result. This is on Linux, don't know if other platforms are affected.

Comment 1

[Jonathan Kamens]

Regression happened between 2019-04-18 and 2019-04-19 nightly builds.

Comment 2

[Jonathan Kamens]

Ping?

Comment 3

[Jonathan Kamens]

An error occurred executing the cmd_fullZoomEnlarge command: ReferenceError: gBrowser is not defined globalOverlay.js:83
goDoCommand chrome://global/content/globalOverlay.js:83
oncommand chrome://messenger/content/messenger.xul:1

Comment 4

[Jonathan Kamens]

https://hg.mozilla.org/mozilla-central/rev/c263feabee1dd5eafa9cbea373e451cb5b9cbf3a

Comment 5

[Jorg K (CEST = GMT+2)]

Aceman, Geoff, any time for this?

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, chrome://global/content/globalOverlay.js:83 merely is reporting the error:
Cu.reportError("An error occurred executing the " + aCommand + " command: " + e);

Pressing Ctrl+"+" on the compose windows gives something similar:

ReferenceError: gBrowser is not defined
viewZoomOverlay.js:36:5
    get zoom chrome://global/content/viewZoomOverlay.js:36
    ZoomManager_enlarge chrome://global/content/viewZoomOverlay.js:80
    doCommand chrome://messenger/content/messengercompose/MsgComposeCommands.js:868
    doCommand chrome://messenger/content/messengercompose/MsgComposeCommands.js:922
    goDoCommand chrome://global/content/globalOverlay.js:81
    oncommand chrome://messenger/content/messengercompose/messengercompose.xul:1

An error occurred executing the cmd_fullZoomEnlarge command: [Exception... "[JavaScript Error: "gBrowser is not defined" {file: "chrome://global/content/viewZoomOverlay.js" line: 36}]'[JavaScript Error: "gBrowser is not defined" {file: "chrome://global/content/viewZoomOverlay.js" line: 36}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 81"  data: yes] globalOverlay.js:83
    goDoCommand chrome://global/content/globalOverlay.js:83
    oncommand chrome://messenger/content/messengercompose/messengercompose.xul:1

Looks like we need to supply gBrowser instead of a getBrowser() function like here:
https://searchfox.org/comm-central/rev/e688c8e64f4bdbe17ea8644f2dd32704eaab9b8d/mail/base/content/mailWindow.js#562

Comment 7

[Benjamin Flanagin]

The bug seems to only affect hotkeys and menu options. Using ctrl+<mouse wheel> works fine. In the most recent Daily (05-06-2019). It may not be a clue but its something. And it does give a work around for those that need the functionality while we track down the issue.

Comment 8

[Jorg K (CEST = GMT+2)]

The issue has been tracked down by the reporter, see comment #4 and further info in comment #6. I'm just not familiar with with that gBrowser / getBrowser() stuff, otherwise I'd have proposed a fix.

It also doesn't look so simple since getBrowser() is evaluated when necessary, or example here:
https://searchfox.org/comm-central/rev/e688c8e64f4bdbe17ea8644f2dd32704eaab9b8d/mail/base/content/mailWindow.js#562

function getBrowser() {
  let tabmail = document.getElementById("tabmail");
  return tabmail ? tabmail.getBrowserForSelectedTab() : getMessagePaneBrowser();
}

... and doesn't have a fixed value in gBrowser. Maybe we need some sort of observer that keeps gBrowser up to date. I tried adding gBrowser = getMessagePaneBrowser(); in that file and that shifted the error elsewhere.

Comment 9

[:aceman]

Can we use some JS hack to define the variable to be called 'gBrowser' but it still runs a function when called, thus some kind of getter on the global scope?
Something like:
Object.defineProperty(this???, "gBrowser", { get() { return getBrowser(); }});

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1546075-gBrowser.patch

This works like a charm on the main window and the compose window. I put the patch in Aceman's name. Aceman, how do you want to proceed here? Review from whom?

EDIT: Also tested on working on stand-alone message window.

Comment 11

[:aceman]

I got the idea from https://bug1515877.bmoattachments.org/attachment.cgi?id=9063405 without testing if it works. If it does, I would still like to hear from arai if it is good to define it on 'this', or there is any more proper way.

Comment 12

[:aceman]

Other candidates may be 'top', 'window', etc.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 9064277 [details] [diff] [review]
1546075-gBrowser.patch

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

looks fine

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/158875588509
fix zooming by providing 'gBrowser' variable as property of the global object. r=jorgk,arai DONTBUILD

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9064277 [details] [diff] [review]
1546075-gBrowser.patch

The landed version differs in formatting.

Comment 16

[:aceman]

Comment on attachment 9064277 [details] [diff] [review]
1546075-gBrowser.patch

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

I'm happy that it works. Thanks Jorg for turning it into a patch and testing the idea.

Comment 17

[Geoff Lankow (:darktrojan)]

I'm not happy that it killed marionette and the mochitest suite. I think we be better if we forked viewZoomOverlay.js as it was when it worked. It's a simple file that wouldn't need to be kept up-to-date with the Firefox version. They hardly ever make changes to it anyway, but when they do … we end up here.

Comment 18

[Geoff Lankow (:darktrojan)]

We either have to make changes to the mozilla-central code that runs marionette, or come up with a new solution here. Either way we should not have a broken test suite in the meantime, so I've backed out this change.

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1ad4306a594a
Backed out changeset 158875588509 for breaking the mochitest suite

Comment 20

[Magnus Melin [:mkmelin]]

For reference, the mochitest failures are like https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=246222717

[task 2019-05-13T23:08:56.816Z] 23:08:56 INFO - Traceback (most recent call last):
[task 2019-05-13T23:08:56.816Z] 23:08:56 INFO - File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 2834, in doTests
[task 2019-05-13T23:08:56.816Z] 23:08:56 INFO - e10s=options.e10s
[task 2019-05-13T23:08:56.816Z] 23:08:56 INFO - File "/builds/worker/workspace/build/tests/mochitest/runtests.py", line 2270, in runApp
[task 2019-05-13T23:08:56.816Z] 23:08:56 INFO - self.marionette.start_session()
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 36, in _
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - m._handle_socket_failure()
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 650, in _handle_socket_failure
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - reraise(exc, val, tb)
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 26, in _
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - return func(*args, **kwargs)
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1116, in start_session
[task 2019-05-13T23:08:56.817Z] 23:08:56 INFO - resp = self._send_message("WebDriver:NewSession", capabilities)
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 36, in _
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - m._handle_socket_failure()
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 650, in _handle_socket_failure
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - reraise(exc, val, tb)
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/decorators.py", line 26, in _
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - return func(*args, **kwargs)
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 590, in _send_message
[task 2019-05-13T23:08:56.818Z] 23:08:56 INFO - msg = self.client.request(name, params)
[task 2019-05-13T23:08:56.819Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/transport.py", line 273, in request
[task 2019-05-13T23:08:56.819Z] 23:08:56 INFO - return self.receive()
[task 2019-05-13T23:08:56.819Z] 23:08:56 INFO - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/transport.py", line 183, in receive
[task 2019-05-13T23:08:56.819Z] 23:08:56 INFO - raise socket.timeout("Connection timed out after {}s".format(self.socket_timeout))
[task 2019-05-13T23:08:56.819Z] 23:08:56 INFO - timeout: Connection timed out after 360s
[task 2019-05-13T23:08:56.819Z] 23:08:56 ERROR - Automation Error: Received unexpected exception while running application

A gBrowser might be needed for bug 1529205 too.

Comment 21

[:aceman]

How do we know from that generic error that it is caused by this patch?
But yet, backing this patch out fixed those tests.

So why is it failing? Where is the real error message, about gBrowser failing?

Comment 22

[Jorg K (CEST = GMT+2)]

For the record: Geoff pointed out that providing gBrowser to the window (this) in the patch, triggers unexpected behaviour here:
https://hg.mozilla.org/mozilla-central/rev/f9d8c6059114#l2.10

I don't quite understand this, since if window.gBrowser is used, we'll return the value of getBrowser() which is

function getBrowser() {
  let tabmail = document.getElementById("tabmail");
  return tabmail ? tabmail.getBrowserForSelectedTab() : getMessagePaneBrowser();
}

That should work unless perhaps the getMessagePaneBrowser() part is executed.

The suggestion to fork viewZoomOverlay.js also seems feasible since it's a small file.

Comment 23

[Geoff Lankow (:darktrojan)]

Here's the code that's failing (in the changeset where I added Thunderbird to marionette): https://hg.mozilla.org/mozilla-central/rev/f9d8c6059114#l2.10

Part of the problem is that gBrowser in Firefox refers to the whole window (vaguely analogous to <tabmail> in Thunderbird) whereas we're trying to perform zoom operations on a specific <browser> element that displays a document. If we taught tabmail to handle zoom and pass it on to the right <browser> (ie. add fullZoom and textZoom getters and setters) we could use it for gBrowser. I think it's probably easier to fork though.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1546075-fork.patch

Here's your fork. Works on main window, message compose and stand-alone window.

Comment 25

[:aceman]

That code is strange:
// Firefox
} else if ("gBrowser" in window) {
return window.gBrowser;
+

• // Thunderbird
• } else if (window.document.getElementById("tabmail")) {
• return window.document.getElementById("tabmail");
  }

gBrowser returns a browser inside tabmail, but the other branch returns just tabmail. Those are different objects as you say.\

Do our marionette tests expect browser.getTabBrowser to return tabmail and not browser as in Firefox? Is that right?
If we wanted, we could check for Thunderbird before the gBrowser in testing/marionette/browser.js, if that would pass through m-c review :)

Comment 26

[Geoff Lankow (:darktrojan)]

Comment on attachment 9064698 [details] [diff] [review]
1546075-fork.patch

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

Ideally you would put the new file in mail/base/content as it's not common (and SeaMonkey already has a copy), but because one of the users is also in common/src, leave it where it is.

::: common/src/viewZoomOverlay.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* import-globals-from ../../mail/base/content/mailWindow.js */

This should be /* globals getBrowser */, since it doesn't necessarily come from the mail window.

Comment 27

[:aceman]

Ah you say gBrowser in Firefox isn't a <browser> object but also a tab manager similar to our tabmail.
But their gBrowser can forward zoom events to the right tab/browser but our tabmail can't so we just return the right browser directly and that can handle zoom events.

Comment 28

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #25)

Do our marionette tests expect browser.getTabBrowser to return tabmail and not browser as in Firefox? Is that right?

Marionette expects the result of getTabBrowser to do tab-handling things, like opening and closing tabs.

If we wanted, we could check for Thunderbird before the gBrowser in testing/marionette/browser.js, if that would pass through m-c review :)

We could, and I think it would pass review if we made a case, but I think a simple fork is less of a hack. Besides, we don't know what other consequences there might be of having a gBrowser object.

Comment 29

[Magnus Melin [:mkmelin]]

Besides, we don't know what other consequences there might be of having a gBrowser object.

Well we're probably going to need one sooner or later. The change that caused this bug was to just use gBrowser instead of calling getBrowser().

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/57e51cfa9101
Fork viewZoomOverlay.js to common/ to fix zooming bug. r=darktrojan DONTBUILD