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

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Masatoshi, could you take your patch in bug 1352773 comment 12 here?

Comment 2

[Masatoshi Kimura [:emk]]

I'll take a look.

Comment 3

[Masatoshi Kimura [:emk]]

Keeping ni? for now.

Comment 4

[Masatoshi Kimura [:emk]]

(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 :(

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

(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.

Comment 6

[Masatoshi Kimura [:emk]]

(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.

Comment 7

[Masatoshi Kimura [:emk]]

Eventually I could make a working patch.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Bug 1353493 - Cache the return value for nsIWidget::DefaultScaleOverride().

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

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

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

https://reviewboard.mozilla.org/r/126788/#review129460

Comment 10

[:Felipe Gomes (needinfo for replies!)]

hah, I had just found this issue through bug 1352525 and was about to file a bug for it!

Comment 11

[Masatoshi Kimura [:emk]]

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

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 13

[Masatoshi Kimura [:emk]]

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.

Comment 14

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dec1d1fb31faa2cb17fa086c1a3012ed01bb78d

Comment 15

[Masatoshi Kimura [:emk]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126788/diff/1-2/

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

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

https://reviewboard.mozilla.org/r/126788/#review130010

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0792bb960b6
Cache the return value for nsIWidget::DefaultScaleOverride(). r=kanru

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Carsten Book [:Tomcat]]

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