nsIWidget::DefaultScaleOverride() should cache layout.css.devPixelsPerPx

RESOLVED FIXED in Firefox 55

Status

()

Core
Widget
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: kanru, Assigned: emk)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: tpi:+)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
nsIWidget::DefaultScaleOverride() is used by nsIWidget::GetDefaultScale() and thus used by TabChild::RecvRealMouseButtonEvent, TabChild::DispatchWheelEvent, TabChild::RecvRealTouchEvent, etc.

I think we should cache the preference variable here.
(Reporter)

Comment 1

3 months ago
Masatoshi, could you take your patch in bug 1352773 comment 12 here?
Flags: needinfo?(VYV03354)
(Assignee)

Comment 2

3 months ago
I'll take a look.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 3

3 months ago
Keeping ni? for now.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 4

3 months ago
(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #1)
> Masatoshi, could you take your patch in bug 1352773 comment 12 here?

Even this patch alone didn't work :(
Flags: needinfo?(VYV03354)
(Reporter)

Comment 5

3 months ago
(In reply to Masatoshi Kimura [:emk] from comment #4)
> (In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #1)
> > Masatoshi, could you take your patch in bug 1352773 comment 12 here?
> 
> Even this patch alone didn't work :(

How come? Can you elaborate? I tried something with Preferences::AddFloatVarCache and it seems to work. I can upload a patch later.
(Assignee)

Comment 6

3 months ago
(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #5)
> (In reply to Masatoshi Kimura [:emk] from comment #4)
> > Even this patch alone didn't work :(
> 
> How come? Can you elaborate? I tried something with
> Preferences::AddFloatVarCache and it seems to work. I can upload a patch
> later.

If I change the pref via about:config, the browser UI does not change accordingly.
(Assignee)

Comment 7

3 months ago
Eventually I could make a working patch.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 9

3 months ago
mozreview-review
Comment on attachment 8854835 [details]
Bug 1353493 - Cache the return value for nsIWidget::DefaultScaleOverride().

https://reviewboard.mozilla.org/r/126788/#review129460
Attachment #8854835 - Flags: review?(kchen) → review+
hah, I had just found this issue through bug 1352525 and was about to file a bug for it!
Blocks: 1352525

Updated

3 months ago
Whiteboard: tpi:+
(Assignee)

Comment 11

3 months ago
Hm, this patch caused xpcshell-test failures in debug builds.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa0d89de6df69b3e427c3017753ab29c74c6542
(Reporter)

Comment 12

3 months ago
The error is

 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:12513) "#01: nsObserverService::RemoveObserver(nsIObserver*, char const*) (/home/kanru/mozilla/mozilla-unified/xpcom/ds/nsObserverService.cpp:233 (discriminator 3))"
 0:02.06 PROCESS_OUTPUT: Thread-1 (pid:12513) "#02: nsCOMPtr<nsIObserverService>::~nsCOMPtr() (/home/kanru/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:402)"
 0:02.06 PROCESS_OUTPUT: Thread-1 (pid:12513) "#03: operator delete(void*) (/home/kanru/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:218 (discriminator 5))"
 0:02.06 PROCESS_OUTPUT: Thread-1 (pid:12513) "#04: mozilla::ClearOnShutdown_Internal::PointerClearer<mozilla::StaticRefPtr<nsIObserver> >::Shutdown() (/home/kanru/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ClearOnShutdown.h:83)"
 0:02.07 PROCESS_OUTPUT: Thread-1 (pid:12513) "#05: mozilla::KillClearOnShutdown(mozilla::ShutdownPhase) (/home/kanru/mozilla/mozilla-unified/xpcom/base/ClearOnShutdown.cpp:38)"
 0:02.08 PROCESS_OUTPUT: Thread-1 (pid:12513) "#06: mozilla::ShutdownXPCOM(nsIServiceManager*) (/home/kanru/mozilla/mozilla-unified/xpcom/build/XPCOMInit.cpp:944)"
 0:02.09 PROCESS_OUTPUT: Thread-1 (pid:12513) "#07: NS_ShutdownXPCOM (/home/kanru/mozilla/mozilla-unified/xpcom/build/XPCOMInit.cpp:828)"
 0:02.12 PROCESS_OUTPUT: Thread-1 (pid:12513) "#08: XRE_XPCShellMain(int, char**, char**, XREShellData const*) (/home/kanru/mozilla/mozilla-unified/js/xpconnect/src/XPCShellImpl.cpp:1640)"
 0:02.15 PROCESS_OUTPUT: Thread-1 (pid:12513) "#09: mozilla::BootstrapImpl::XRE_XPCShellMain(int, char**, char**, XREShellData const*) (/home/kanru/mozilla/mozilla-unified/toolkit/xre/Bootstrap.cpp:54)"
 0:02.17 PROCESS_OUTPUT: Thread-1 (pid:12513) "#10: main (/home/kanru/mozilla/mozilla-unified/js/xpconnect/shell/xpcshell.cpp:68)"
 0:02.23 PROCESS_OUTPUT: Thread-1 (pid:12513) "#11: __libc_start_main (/build/glibc-bDJTDn/glibc-2.24/csu/../csu/libc-start.c:325)"
 0:02.23 PROCESS_OUTPUT: Thread-1 (pid:12513) "#12: _start (/home/kanru/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell)"
 0:02.23 PROCESS_OUTPUT: Thread-1 (pid:12513) "#13: ??? (???:???)"
 0:02.23 PROCESS_OUTPUT: Thread-1 (pid:12513) "[12513] ###!!! ASSERTION: Using observer service after XPCOM shutdown!: 'Error', file /home/kanru/mozilla/mozilla-unified/xpcom/ds/nsObserverService.cpp, line 233"
 0:02.23 PROCESS_OUTPUT: Thread-1 (pid:12513) "Hit MOZ_CRASH() at /home/kanru/mozilla/mozilla-unified/memory/mozalloc/mozalloc_abort.cpp:33"

Apparently you can't remove the observer in the destructor because the observer still holds a strong reference to it, sorry I didn't catch that in the review. You can use observe the xpcom-shutdown topic and do the cleanup in the handler.
(Assignee)

Comment 13

3 months ago
Oh, we don't have to manually call RemoveObserver at all because the observer service will remove all observers on shutdown. We don't even have to hold the observer on our own because the observer service has a strong ref to the observer. Also we should not call AddObserver in the const6ructor where the refcount is still zero. Patch is coming.
(Assignee)

Comment 14

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dec1d1fb31faa2cb17fa086c1a3012ed01bb78d
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8854835 - Flags: review+ → review?(kchen)
(Reporter)

Comment 16

3 months ago
mozreview-review
Comment on attachment 8854835 [details]
Bug 1353493 - Cache the return value for nsIWidget::DefaultScaleOverride().

https://reviewboard.mozilla.org/r/126788/#review130010
Attachment #8854835 - Flags: review?(kchen) → review+

Comment 17

3 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/f0792bb960b6
Cache the return value for nsIWidget::DefaultScaleOverride(). r=kanru

Comment 18

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0792bb960b6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 19

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0792bb960b6
(Assignee)

Updated

3 months ago
Depends on: 1356565
You need to log in before you can comment on or make changes to this bug.