1475875 - Screen metrics like screen.width, screen.height, and @media return bogus zeros in GeckoView e10s

Status: RESOLVED FIXED

(GeckoView :: Media, defect, P1)

Description

[Jan Henning [:JanH]]

Came across this while implementing bug 1460874 and testing with the GeckoView example app.

If I enable font inflation, text that is subject to font inflation is displayed much too large if the multiprocess setting in the example app is enabled.

For an example page that also contains some elements not subject to font inflation (the header and some images), see http://freefall.purrsia.com/.

As far as I can tell, trying different font size factors (which with the implementation from bug 1460874 results in setting correspondingly different values for the font.size.inflation.minTwips pref, which is what actually controls the amount of font inlation) doesn't seem to result in any appreciable size difference of the inflated text when compared to non-inflated page elements.

Comment 1

[Jan Henning [:JanH]]

I'm not sure how far I'll be able to get in debugging this, but so far I've found that under e10s, aPresContext->ScreenSizeInchesForFontInflation() at [1] returns 0.00 x 0.00 inches, which naturally messes up the minimum font size calculation.


[1] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/base/nsLayoutUtils.cpp#8303

Comment 2

[Jan Henning [:JanH]]

It looks like bug 1194751 might be to blame here: As per bug 1194751 comment 10, the idea of that bug was to
> 2) Have each ContentChild singleton receive and store a cache of the
> properties of each screen that is available on the system. If the parent
> process notices that a new monitor has been added or a screen's dimensions
> have changed*, then that cache will need to be invalidated.

Apparently this was never implemented for Android, which would explain why querying any screen properties from a content process on Android results in bogus values.

The silver lining is that I was already thinking about somehow caching the screen size within the content process, as ScreenSizeInchesForFontInflation() is called quite a lot during page loading if font inflation is active for that page. It looks like bug 1194751 has already done most of the required work for that and we only need to start providing (and invalidating as appropriately) the required data on Android as well.

Comment 3

[Chris Peterson [:cpeterson]]

Jan, are you interested in fixing this problem? GeckoView does not expose an API to enable font inflation yet. We haven't decided whether to enable font inflation for GeckoView-based apps.

Comment 4

[Jan Henning [:JanH]]

I might be able to look into it, but not sure how soon.

> GeckoView does not expose an API to enable font inflation yet.
The keyword possibly being "yet"? (Bug 1460874)

> We haven't decided whether to enable font inflation for GeckoView-based apps.
Isn't there an issue for bringing Fennec's "Use system font size" system to Focus?

Plus I'd like to remind that Chrome still offers font inflation by default.

Comment 6

[Chris Peterson [:cpeterson]]

Bumping priority to P1 because the root cause of this bug is about screen metrics returning. Jan says this bug is about more than font inflation.

In bug 1478376, Andreas provided some STR:

1. Go to http://mydevice.io with Focus (GeckoView) and inspect listed values.

Result:
screen.width and screen.height values are 0px, @media values are empty, and Device Aspect-Ratio returns NaN.

Expected:
Fennec behaves correctly. This works fine in Fennec Custom Tab with GeckoView as well.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1475875 - Use ScreenManager on Android

This makes sure things work under e10s, but also results
in simplified code since we weren't actually using the
features of nsScreenManagerAndroid any more.

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

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review267094

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46cf39b31988
Use ScreenManager on Android r=esawin

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/46cf39b31988

Comment 11

[Chris Peterson [:cpeterson]]

James, should we uplift your fix to 62 Beta? Is this code shared with Fennec and non-e10s?

Comment 12

[Jan Henning [:JanH]]

Bug 1479714 should be sorted out first, plus this is actually only a partial fix - there's further work needed that is tracked by bug 1476106 (I should have a patch up within the next few days).

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ddd541d148d
Backed out due to Chromecast breakage. r=backout

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

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

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review267882

::: widget/android/ScreenHelperAndroid.h:12
(Diff revision 2)
> +#ifndef ScreenHelperAndroid_h___
> +#define ScreenHelperAndroid_h___
> +
> +#include "mozilla/widget/ScreenManager.h"
> +
> +using namespace mozilla::widget;

`ScreenHelperAndroid` should be under `mozilla::widget` namespace (and headers shouldn't have `using namespace` for that matter).

::: widget/android/ScreenHelperAndroid.h:17
(Diff revision 2)
> +using namespace mozilla::widget;
> +
> +class ScreenHelperAndroid final : public ScreenManager::Helper
> +{
> +public:
> +    class ScreenHelperSupport;

Need to call `ScreenHelperSupport::Init()` somewhere (or merge `ScreenHelperSupport` into `ScreenHelperAndroid`)

::: widget/android/ScreenHelperAndroid.h:27
(Diff revision 2)
> +
> +    static ScreenHelperAndroid* GetSingleton();
> +
> +    void Refresh();
> +
> +    std::pair<uint32_t, already_AddRefed<Screen>> AddScreen(DisplayType aDisplayType,

I think we're supposed to use `mozilla::Pair`

::: widget/android/ScreenHelperAndroid.h:34
(Diff revision 2)
> +                                       float aDensity = 1.0f);
> +    void RemoveScreen(uint32_t aId);
> +    already_AddRefed<Screen> ScreenForId(uint32_t aId);
> +
> +private:
> +    std::map<uint32_t, RefPtr<Screen>> mScreens;

Feels like we should keep using `nsTArray` because most of the time there is no additional screen or just one screen.

::: widget/android/ScreenHelperAndroid.cpp:32
(Diff revision 2)
> +    typedef ScreenManagerHelper::Natives<ScreenHelperSupport> Base;
> +
> +    static int32_t AddDisplay(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity) {
> +        int32_t screenId = -1; // return value
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        SyncRunnable::DispatchToThread(mainThread, NS_NewRunnableFunction(

No need to block the calling thread (UI thread?). We should preallocate an ID and add a screen on the Gecko thread using that ID.

::: widget/android/ScreenHelperAndroid.cpp:51
(Diff revision 2)
> +        return screenId;
> +    }
> +
> +    static void RemoveDisplay(int32_t aScreenId) {
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        SyncRunnable::DispatchToThread(mainThread, NS_NewRunnableFunction(

Same, no need to block

::: widget/android/ScreenHelperAndroid.cpp:84
(Diff revision 2)
> +}
> +
> +ScreenHelperAndroid::ScreenHelperAndroid()
> +{
> +    MOZ_ASSERT(!gHelper);
> +    gHelper = this;

Need to unset `gHelper` in destructor. I think a better way is to expose `GetHelper` in `ScreenManager` though.

::: widget/android/ScreenHelperAndroid.cpp:116
(Diff revision 2)
> +std::pair<uint32_t, already_AddRefed<Screen>>
> +ScreenHelperAndroid::AddScreen(DisplayType aDisplayType,
> +                               LayoutDeviceIntRect aRect,
> +                               float aDensity)
> +{
> +    static uint32_t nextId = 1;

`nextId = 0` is a bit more efficient

::: widget/android/nsWidgetFactory.cpp:35
(Diff revision 2)
> +using namespace mozilla;
> +using namespace mozilla::widget;

Just use `mozilla::widget::ScreenManager`

::: widget/android/nsWidgetFactory.cpp:102
(Diff revision 2)
>  
>  static const mozilla::Module::CIDEntry kWidgetCIDs[] = {
>    { &kNS_WINDOW_CID, false, nullptr, nsWindowConstructor },
>    { &kNS_CHILD_CID, false, nullptr, nsWindowConstructor },
>    { &kNS_APPSHELL_CID, false, nullptr, nsAppShellConstructor },
> -  { &kNS_SCREENMANAGER_CID, false, nullptr, nsScreenManagerAndroidConstructor },
> +  { &kNS_SCREENMANAGER_CID, false, NULL, ScreenManagerConstructor,

`nullptr` please

::: widget/android/nsWindow.cpp:1534
(Diff revision 2)
> -        return AndroidBridge::Bridge()->GetDPI();
> +    if (!screen) {
> -    return 160.0f;
> +        return 160.0f;
> -}
> +    }
>  
> +    float dpi;
> +    screen->GetDpi(&dpi);

`float dpi = 160.0f; if (screen) screen->GetDpi(&dpi);` because technically `GetDpi` can fail.

::: widget/android/nsWindow.cpp:1545
(Diff revision 2)
> -
>      nsCOMPtr<nsIScreen> screen = GetWidgetScreen();
>      MOZ_ASSERT(screen);
> -    RefPtr<nsScreenAndroid> screenAndroid = (nsScreenAndroid*) screen.get();
> -    return screenAndroid->GetDensity();
> +
> +    double scale;
> +    screen->GetContentsScaleFactor(&scale);

Same, `GetContentsScaleFactor` can technically fail.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268094

::: widget/android/ScreenHelperAndroid.h:34
(Diff revision 2)
> +                                       float aDensity = 1.0f);
> +    void RemoveScreen(uint32_t aId);
> +    already_AddRefed<Screen> ScreenForId(uint32_t aId);
> +
> +private:
> +    std::map<uint32_t, RefPtr<Screen>> mScreens;

We need a place to store the id. Array indices don't work because you could remove in non-fifo order. Putting the ID in nsIScreen is cumbersome. This seemed to be the least-intrusive thing.

::: widget/android/ScreenHelperAndroid.cpp:32
(Diff revision 2)
> +    typedef ScreenManagerHelper::Natives<ScreenHelperSupport> Base;
> +
> +    static int32_t AddDisplay(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity) {
> +        int32_t screenId = -1; // return value
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        SyncRunnable::DispatchToThread(mainThread, NS_NewRunnableFunction(

I just pasted what was already there before, but yeah that sounds fine.

::: widget/android/ScreenHelperAndroid.cpp:116
(Diff revision 2)
> +std::pair<uint32_t, already_AddRefed<Screen>>
> +ScreenHelperAndroid::AddScreen(DisplayType aDisplayType,
> +                               LayoutDeviceIntRect aRect,
> +                               float aDensity)
> +{
> +    static uint32_t nextId = 1;

0 is reserved for the primary screen, though I wasn't adding it to the map. I've fixed that now.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

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

Comment 18

[Jan Henning [:JanH]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268114

::: widget/android/ScreenHelperAndroid.cpp:124
(Diff revision 3)
> +                               DisplayType aDisplayType,
> +                               LayoutDeviceIntRect aRect,
> +                               float aDensity)
> +{
> +    MOZ_ASSERT(aScreenId > 0);
> +    MOZ_ASSERT(mScreen.count(aScreenId) == 0);

`mScreens`

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268120

r- only because of the `ScreenHelperAndroid::Refresh` and `ScreenForId` issues. Looks good otherwise.

::: widget/android/ScreenHelperAndroid.h:35
(Diff revision 3)
> +                   float aDensity = 1.0f);
> +    void RemoveScreen(uint32_t aId);
> +    already_AddRefed<Screen> ScreenForId(uint32_t aScreenId);
> +
> +private:
> +    std::map<uint32_t, RefPtr<Screen>> mScreens;

I was thinking `nsTArray<Pair<const uint32_t, RefPtr<Screen>>` to hold the ID but whatever. In any case I think we are supposed to use `nsDataHashtable` instead of `std::map`.

::: widget/android/ScreenHelperAndroid.cpp:29
(Diff revision 3)
> +{
> +public:
> +    typedef ScreenManagerHelper::Natives<ScreenHelperSupport> Base;
> +
> +    static int32_t AddDisplay(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity) {
> +        static int32_t nextId = 1;

`uint32_t`. Also I'd prefer `nextId = 0` and then `++nextId`.

::: widget/android/ScreenHelperAndroid.cpp:31
(Diff revision 3)
> +    typedef ScreenManagerHelper::Natives<ScreenHelperSupport> Base;
> +
> +    static int32_t AddDisplay(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity) {
> +        static int32_t nextId = 1;
> +
> +        int32_t screenId = nextId++;

This is not thread safe

::: widget/android/ScreenHelperAndroid.cpp:34
(Diff revision 3)
> +        static int32_t nextId = 1;
> +
> +        int32_t screenId = nextId++;
> +        NS_DispatchToMainThread(NS_NewRunnableFunction(
> +            "ScreenHelperAndroid::ScreenHelperSupport::AddDisplay",
> +            [&aDisplayType, &aWidth, &aHeight, &aDensity, &screenId] {

Pass by value

::: widget/android/ScreenHelperAndroid.cpp:36
(Diff revision 3)
> +        int32_t screenId = nextId++;
> +        NS_DispatchToMainThread(NS_NewRunnableFunction(
> +            "ScreenHelperAndroid::ScreenHelperSupport::AddDisplay",
> +            [&aDisplayType, &aWidth, &aHeight, &aDensity, &screenId] {
> +                MOZ_ASSERT(NS_IsMainThread());
> +

Extra line

::: widget/android/ScreenHelperAndroid.cpp:110
(Diff revision 3)
> +        screenList.AppendElement(screen);
> +        mScreens[0] = screen;
> +    }
> +
> +    for (auto const& it : mScreens) {
> +        screenList.AppendElement(it.second);

This adds the primary screen a second time.

::: widget/android/ScreenHelperAndroid.cpp:148
(Diff revision 3)
> +already_AddRefed<Screen>
> +ScreenHelperAndroid::ScreenForId(uint32_t aScreenId)
> +{
> +    auto it = mScreens.find(aScreenId);
> +    if (it != mScreens.end()) {
> +        return it->second.forget();

This clears the mapping. We should return a copy here.

::: widget/android/nsWindow.cpp:1526
(Diff revision 3)
>  nsWindow::GetDPI()
>  {
> -    if (AndroidBridge::Bridge())
> -        return AndroidBridge::Bridge()->GetDPI();
> -    return 160.0f;
> +    float dpi = 160.0f;
> +
> +    nsCOMPtr<nsIScreen> screen = GetWidgetScreen();
> +    MOZ_ASSERT(screen);

Seems like `screen` whould be null if we don't have a primary screen (e.g. xpcshell)

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268120

> I was thinking `nsTArray<Pair<const uint32_t, RefPtr<Screen>>` to hold the ID but whatever. In any case I think we are supposed to use `nsDataHashtable` instead of `std::map`.

Do you know of some canonical place for these recommendations? I see people using both pretty frequently.

> This is not thread safe

I think in practice this is never going to be a problem, but I can make it atomic.

> Seems like `screen` whould be null if we don't have a primary screen (e.g. xpcshell)

Can you open windows with xpcshell?

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260006/diff/3-4/

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268254

r+ with the pass-by-value issues fixed

::: widget/android/ScreenHelperAndroid.cpp:32
(Diff revision 4)
> +    typedef ScreenManagerHelper::Natives<ScreenHelperSupport> Base;
> +
> +    static int32_t AddDisplay(int32_t aDisplayType, int32_t aWidth, int32_t aHeight, float aDensity) {
> +        static Atomic<uint32_t> nextId;
> +
> +        int32_t screenId = ++nextId;

`uint32_t`

::: widget/android/ScreenHelperAndroid.cpp:35
(Diff revision 4)
> +        static Atomic<uint32_t> nextId;
> +
> +        int32_t screenId = ++nextId;
> +        NS_DispatchToMainThread(NS_NewRunnableFunction(
> +            "ScreenHelperAndroid::ScreenHelperSupport::AddDisplay",
> +            [&aDisplayType, &aWidth, &aHeight, &aDensity, screenId] {

Pass by value

::: widget/android/ScreenHelperAndroid.cpp:49
(Diff revision 4)
> +    }
> +
> +    static void RemoveDisplay(int32_t aScreenId) {
> +        NS_DispatchToMainThread(NS_NewRunnableFunction(
> +            "ScreenHelperAndroid::ScreenHelperSupport::RemoveDisplay",
> +            [&aScreenId] {

By value

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268120

> Do you know of some canonical place for these recommendations? I see people using both pretty frequently.

I found a blog post by :nfroyd, https://blog.mozilla.org/nfroyd/2016/05/31/why-gecko-data-structures-should-be-preferred-to-std-ones/

> Can you open windows with xpcshell?

I think so, or at least hidden windows?

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review268306

::: widget/android/ScreenHelperAndroid.cpp:123
(Diff revision 4)
> +                               DisplayType aDisplayType,
> +                               LayoutDeviceIntRect aRect,
> +                               float aDensity)
> +{
> +    MOZ_ASSERT(aScreenId > 0);
> +    MOZ_ASSERT(mScreens.count(aScreenId) == 0);

Not building, `!mScreens.Get(aScreenId, nullptr)` might be the equivalent for hashtables?

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260006/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09871ad4c346
Use ScreenManager on Android r=esawin,jchen

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/09871ad4c346

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Is this something that needs Beta uplift consideration?

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

Approval Request Comment
[Feature/Bug causing the regression]: Using e10s
[User impact if declined]: Busted DOM stuff
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Go to mydevice.io using an e10s-enabled browser using GeckoView (such as the example app or Focus). Ensure the JS screen.width values are non-zero.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No, but does share code with Fennec
[Why is the change risky/not risky?]: Common paths are well tested
[String changes made/needed]: None

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

May be a little risky but it looks like this fixes multiple problems. Let's try it for beta 16/17.

Comment 31

[Andreas Bovens [:abovens]]

I was NIed - anything in particular?

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b9c3989046b

Comment 33

[Chris Peterson [:cpeterson]]

Moving some media bugs to the new GeckoView::Media component.