Closed Bug 1303380 Opened 8 years ago Closed 8 years ago

_firstWindowTelemetry() fails on some Tier3 platforms

Categories

(Firefox :: General, defect)

Unspecified
FreeBSD
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: jbeich, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(1 file)

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)
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.
(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 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+
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
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
Status: ASSIGNED → RESOLVED
Closed: 8 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.

Attachment

General

Created:
Updated:
Size: