_firstWindowTelemetry() fails on some Tier3 platforms

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jbeich, Assigned: jaws)

Tracking

({regression})

Trunk
Firefox 52
Unspecified
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox-esr45 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
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.
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.
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.
Flags: needinfo?(jbeich)
Reporter

Comment 3

3 years ago
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
Flags: needinfo?(jbeich)
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.
Reporter

Comment 6

3 years ago
(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/firefox@getpocket.com/bootstrap.js :: PocketOverlay.startup :: line 351"  data: no] Stack trace: PocketOverlay.startup()@resource://gre/modules/addons/XPIProvider.jsm -> file://dist/bin/browser/features/firefox@getpocket.com/bootstrap.js:351 < startup/<()@resource://gre/modules/addons/XPIProvider.jsm -> file://dist/bin/browser/features/firefox@getpocket.com/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

3 years ago
mozreview-review
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
Attachment #8792633 - Flags: review?(gijskruitbosch+bugs) → review+
Reporter

Comment 8

3 years ago
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.
Attachment #8792633 - Flags: feedback+
Assignee: nobody → jaws
Status: NEW → ASSIGNED

Comment 9

3 years ago
Pushed by jwein@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee364270d3e6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Is it something we are planning to uplift? Thanks
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.
You need to log in before you can comment on or make changes to this bug.