1303380 - _firstWindowTelemetry() fails on some Tier3 platforms

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jan Beich]

After bug 1023508 firefox worked fine with occasional crashes but bug 1241993 made the issue explicit. On one hand, Tier3 platforms (mainly BSDs and Solaris) have the same rendering semantics and UI theme as Linux[1], on the other, DISPLAY_SCALING_* doesn't appear to be exposed in about:telemetry while about:preferences#advanced (Data Choices) is blank.

$ firefox -no-remote -jsconsole -profile $(mktemp -d) about:blank
uncaught exception: undefined  (unknown)
NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsITelemetry.getHistogramById]  nsBrowserGlue.js:934

[1] HiDPI or rather "Xft.dpi: 161" in ~/.Xdefaults appears to scale Firefox UI and web content properly on my 4k display. Intel DDX for some reason always reports 96x96 DPI unlike Nvidia.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

It seems the real bug here is that window.devicePixelRatio for Intel DDX is reporting a string for the DPI (96x96) which cannot be coerced to a Number whereas our code expects a Number.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Did I understand that right from you Jan? Can you see what window.devicePixelRatio returns for you on the platforms where you're seeing this?

I have a simple patch that will work-around this but it doesn't make sense that we would be returning a non-Number here.

Comment 3

[Jan Beich]

Correction, Firefox (or any other Gtk3 app) ignores display DPI if Xft.dpi is unset for either DDX after https://git.gnome.org/browse/gtk+/commit/?id=bdf0820c501437a2150d8ff0d5340246e713f73f . It doesn't affect nsITelemetry error in comment 0.

  # nvidia (367.44)
  $ xdpyinfo | fgrep dots
    resolution:    162x161 dots per inch
  $ fgrep DPI /var/log/Xorg.0.log
  [ 18585.840] (--) NVIDIA(0): DPI set to (162, 161); computed from "UseEdidDpi" X config
  > window.devicePixelRatio
  1

vs.

  # intel (git master)
  $ xdpyinfo | fgrep dots
    resolution:    96x96 dots per inch
  $ fgrep DPI /var/log/Xorg.0.log
  [ 20812.838] (==) intel(0): DPI set to (96, 96)
  > window.devicePixelRatio
  1

vs.

  $ fgrep dpi ~/.Xdefaults
  Xft.dpi:     161
  > window.devicePixelRatio
  1.5

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Okay, thanks for clearing that up. In comment #0, you mention crashing, but an exception here shouldn't cause a crash. It looks like it may prevent migration code from running to completion though.

We can wrap the telemetry call in a try/catch as the simple fix here. The error seems more related to telemetry not being enabled on that build than being related to the actual value reported by window.devicePixelRatio.

This is confirmed by the blank Data Choices section.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1303380 - Wrap the telemetry code in _firstWindowLoaded with a try/catch to allow startup and migration code to run to completion if there is an exception writing to Telemetry.

Review commit: https://reviewboard.mozilla.org/r/79552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79552/

Comment 6

[Jan Beich]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> In comment #0, you mention crashing, but an exception here shouldn't cause a crash.

Sorry, the crash was unrelated and no longer happens on Nightly. If you're curious see below:

  $ ./mach run -jsconsole https://www.youtube.com
   0:00.09 dist/bin/firefox -jsconsole https://www.youtube.com -no-remote -profile tmp/scratch_user
  1474314443696   addons.manager  WARN    Exception calling callback: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIStyleSheetService.preloadSheet]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> file://dist/bin/browser/features/[e-mail]/bootstrap.js :: PocketOverlay.startup :: line 351"  data: no] Stack trace: PocketOverlay.startup()@resource://gre/modules/addons/XPIProvider.jsm -> file://dist/bin/browser/features/[e-mail]/bootstrap.js:351 < startup/<()@resource://gre/modules/addons/XPIProvider.jsm -> file://dist/bin/browser/features/[e-mail]/bootstrap.js:537 < safeCall()@resource://gre/modules/AddonManager.jsm:179 < makeSafe/<()@resource://gre/modules/AddonManager.jsm:195 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812

  (process:50126): GLib-CRITICAL **: g_path_get_basename: assertion 'file_name != NULL' failed
  [xcb] Unknown sequence number while processing queue
  [xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
  [xcb] Aborting, sorry about that.
  Assertion failed: (! xcb_xlib_threads_sequence_lost), function poll_for_event, file xcb_io.c, line 274.
  Redirecting call to abort() to mozalloc_abort

  [Child 50126] ###!!! ABORT: Aborting on channel error.: file ipc/glue/MessageChannel.cpp, line 1822
  [Child 50126] ###!!! ABORT: Aborting on channel error.: file ipc/glue/MessageChannel.cpp, line 1822

Comment 7

[:Gijs (he/him)]

Comment on attachment 8792633 [details]
Bug 1303380 - Wrap the telemetry code in _firstWindowLoaded with a try/catch to allow startup and migration code to run to completion if there is an exception writing to Telemetry.

https://reviewboard.mozilla.org/r/79552/#review78222

Comment 8

[Jan Beich]

Comment on attachment 8792633 [details]
Bug 1303380 - Wrap the telemetry code in _firstWindowLoaded with a try/catch to allow startup and migration code to run to completion if there is an exception writing to Telemetry.

AppConstants.platform == "linux" here. After this change browser console no longer shows errors (at least on about:blank). Thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee364270d3e6
Wrap the telemetry code in _firstWindowLoaded with a try/catch to allow startup and migration code to run to completion if there is an exception writing to Telemetry. r=Gijs

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Is it something we are planning to uplift? Thanks

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

It's not a recent regression (first introduced in v33 and made worse in v46) so I'd say that we shouldn't be concerned with uplift, though it is a very low-risk patch if we did uplift it. My overall answer is that it's not necessary to uplift.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

ok, merci!