Closed Bug 1475875 Opened Last year Closed Last year

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

Categories

(GeckoView :: General, defect, P1)

All
Android
defect

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: JanH, Assigned: snorp)

References

()

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(1 file)

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.
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
See Also: → 1476106
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.
Blocks: 1194751
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.
Priority: -- → P3
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.
Duplicate of this bug: 1478376
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.
Priority: P3 → P1
Summary: Font inflation is broken under e10s → Screen metrics like screen.width, screen.height, and @media return bogus zeros in GeckoView e10s
Whiteboard: [geckoview:klar:p1]
Assignee: nobody → snorp
Comment on attachment 8995602 [details]
Bug 1475875 - Use ScreenManager on Android

https://reviewboard.mozilla.org/r/260006/#review267094
Attachment #8995602 - Flags: review?(esawin) → review+
Blocks: 1476106
See Also: 1476106
https://hg.mozilla.org/mozilla-central/rev/46cf39b31988
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
James, should we uplift your fix to 62 Beta? Is this code shared with Fennec and non-e10s?
Flags: needinfo?(snorp)
Depends on: 1479714
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).
Backout by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ddd541d148d
Backed out due to Chromecast breakage. r=backout
Status: RESOLVED → REOPENED
Flags: needinfo?(snorp)
Resolution: FIXED → ---
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.
Attachment #8995602 - Flags: review?(nchen) → review-
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 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 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)
Attachment #8995602 - Flags: review?(nchen) → review-
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 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
Attachment #8995602 - Flags: review?(nchen) → review+
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 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?
Flags: needinfo?(abovens)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09871ad4c346
Use ScreenManager on Android r=esawin,jchen
https://hg.mozilla.org/mozilla-central/rev/09871ad4c346
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Is this something that needs Beta uplift consideration?
Flags: needinfo?(snorp)
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
Flags: needinfo?(snorp)
Attachment #8995602 - Flags: approval-mozilla-beta?
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.
Attachment #8995602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I was NIed - anything in particular?
Flags: needinfo?(abovens)
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.