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

RESOLVED FIXED in Firefox 55

Status

()

Core
Widget
RESOLVED FIXED
22 days ago
12 days 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

22 days 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

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

Comment 2

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

Comment 3

22 days ago
Keeping ni? for now.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 4

22 days 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

22 days 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

21 days 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

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

Comment 9

21 days 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

21 days ago
Whiteboard: tpi:+
(Assignee)

Comment 11

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

Comment 12

21 days 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

21 days 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

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

Updated

20 days ago
Attachment #8854835 - Flags: review+ → review?(kchen)
(Reporter)

Comment 16

20 days 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

20 days 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

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

Comment 19

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

Updated

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