968991 - OOP Keyboard issues with Nuwa

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[[:fabrice] Fabrice Desré]

We had to disable OOP keyboards to keep Nuwa on. Let's investigate and fix the issues.

Comment 1

[Jason Smith [:jsmith]]

Blocks a 1.4 committed feature - oop keyboard support

Comment 2

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: test_re-enable-oop.patch

As pointed out by bug 963584 comment 28, when this issue occurs, we cannot get correct height of the keyboard because window.innerHeight is 0.

And we have 3 workarounds as follows, while I think #2, #3 are talking about the same thing,
 1. Disable APZ.
 2. or Disable Nuwa.
 3. (the current workaournd) Disable keyboard OOP.

--
BTW, this issue won't occur after I re-enable keyboard OOP and also make keyboard not to be pre-launch at boot time.
See the attachment for the exact setting entries.

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Kats,

Could you help shed some light on why we could get "window.innerHeight == 0" when:
 APZC is on and we launch keyboard app immediately after boot?

Thanks.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

When APZC is on, the innerHeight is computed in TabChild::HandlePossibleViewportChange. Can you check to see if that function is getting run for the keyboard process, and if so, what it passes to SetCSSViewport at [1]?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#550

Comment 5

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: dump log for keyboard viewport change

Before the following timestamp, the viewport size of keyboard is still (0, 0).
Note: The keyboard app is prelaunched after boot, and not triggered by the user yet.
---
02-10 06:42:10.529: I/Gecko(881): early return for no screenW or no screenH


But after I clicked on an input field, we could see that the viewport has been set as (320, 480).
However, the keyboard app still get 0 from window.innerHeight.
--
02-10 06:43:21.229: I/Gecko(881): In TabChild::HandlePossibleViewportChange() 
02-10 06:43:21.229: I/Gecko(881): screenW: 320.000000 screeH: 480.000000 
02-10 06:43:21.229: I/Gecko(881): viewport.width: 320.000000, viewport.height: 480.000000

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Kats,

Could you please help check comment 5 and let us know what else we should look into?
Thank you very much.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

If the viewport is set to a 480 height, then that is what should be coming out of window.innerHeight. If that is not then you'll have to trace into nsGlobalWindow::GetInnerHeight to see why that's not the case.

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: dump logs 2

I found that nsIPresShell::SetScrollPositionClampingScrollPortSize() was called with (0, 0) in TabChild::HandlePossibleViewportChange(), which result the 0 innerHeight.

At the end, I traced back to the following line and found the mZoom would be zero here.
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h#165
--
02-11 22:12:32.319: I/Gecko(2740): FrameMetrics::CalculateCompositedRectInCssPixels(), width: 320, height: 480, zoom: 0.000000

You may find the full log of both failure or success cases in the attachment.

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: keyboard dump logs with apz info

This dump also contains the APZ debug logs, but seems before APZ update anything about mZoom, mZoom is already 0.

Kats, could you help take a look what else should we look at to know how mZoom was set for FrameMetrics?

Thanks.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Hm, interesting. metrics.mZoom is updated just a little farther up in HandlePossibleViewportChange:

metrics.mZoom.scale *= metrics.CalculateIntrinsicScale().scale / oldIntrinsicScale;

which seems to imply that either metrics.mZoom was already 0, or metrics.CalculateIntrinsicScale().scale is 0, which in turn means that the composition bounds width is zero. But that's obviously not the case from your logs. So it seems probably that metrics.mZoom was 0 before this, which means mLastRootMetrics.mZoom was 0, which goes all the way back to mInnerSize being zero at the time the before-first-paint event was dispatched.

Can you verify that mInnerSize is 0 inside the BEFORE_FIRST_PAINT handler where mLastRootMetrics is initialized? That seems like an underlying bug to me but I could be wrong - that might be normal. In which case we should just update this block of code:

  float oldScreenWidth = mLastRootMetrics.mCompositionBounds.width;
  if (!oldScreenWidth) {
    oldScreenWidth = mInnerSize.width;
  }

to something like this:

  if (!mLastRootMetrics.mCompositionBounds.width) {
    mLastRootMetrics.mCompositionBounds.SizeTo(mInnerSize);
    mLastRootMetrics.mZoom = mLastRootMetrics.CalculateIntrinsicScale();
  }
  float oldScreenWidth = mLastRootMetrics.mCompositionBounds.width;

and hopefully that will avoid the problem. Please give that a shot and let me know if it works.

Comment 11

[Cervantes Yu [:cyu] [:cervantes]]

I tried to apply the change and it works.

Comment 12

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Cervantes Yu from comment #11)
> I tried to apply the change and it works.
Hmm, it works "to some extent". The keyboard sometimes works, sometimes not.

Comment 13

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: WIP: an incorrect fix

This is not the correct way to fix the problem, but works the problem around.

Comment 14

[Cervantes Yu [:cyu] [:cervantes]]

We found that TabChild::RecvUpdateDimensions() happens after TabChild::Observe() that calculates mLastRootMetrics.mZoom. If we can update mZoom when we really needs to show, then the keyboard app works. This looks to be a timing issue. :kats, would you take a look at the WIP and work on the right fix?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

You said that the change from comment 11 works sometimes, but that your patch in comment 13 always works? That seems odd to me, because every time your code in comment 13 gets hit, the code path in comment 11 should also get hit, because RecvUpdateDimensions calls HandlePossibleViewportChange. The only real difference between the two patches is that mine checks for a zero composition bounds and yours does not. I guess it might be that ProcessUpdateFrame is getting called between the before-first-paint handler and the RecvUpdateDimensions, and that is setting the composition bounds to something else; but if that is the case the zoom should also be getting set to something else.

But yeah I guess I can look at this. Can you give me full steps to reproduce? Do I just need to apply the patch in comment 2, or is there something else that needs to be done as well?

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Renable keyboard OOP in Gaia

Hi Kats,

Please try this patch to re-enable keyboard OOP in Gaia, it should be easy to reproduce this issue, just click any input field after the keyboard process is already launched, and then you would see the keyboard won't show up.

Thanks for your help on this issue.

Comment 17

[Jason Smith [:jsmith]]

Moving to 1.5? as third party keyboard is moving out of scope for 1.4

Comment 18

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> You said that the change from comment 11 works sometimes, but that your
> patch in comment 13 always works?

I tested both patches today and yes, patch in comment 11 works sometimes and the one from comment 13 works always.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Debugging log patch

Unfortunately I'm unable to reproduce this on my Peak device on master. I tried both debug and non-debug builds and in both cases the keyboard seems to work fine. Can you apply this patch and reproduce the problem and post the logcat?

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Oh, my mistake. I used the old patch to re-enable the keyboard, which had some other pref change as well. With the right patch applied I can repro this problem.

Comment 21

[(not reading bugmail) Nick Desaulniers [:\n]]

Jason, how many times is third party keyboard support going to be pushed to the next version?  I have developers anxiously waiting to develop third party keyboards.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This patch seems to fix the problem reliably. I'm not that confident in my knowledge of this code though so it may have bad side effects.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: Updated logging patch

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Cervantes and/or Rudy, can you apply the patch from attachment 8379120 [details] [diff] [review] to see if it reliably fixes the problem for you? If not, can you also apply the updated logging patch and get a full log from startup to the keyboard not showing? Thanks.

Comment 25

[Cervantes Yu [:cyu] [:cervantes]]

This reliably fixes the problem on my unagi.

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8379120 [details] [diff] [review]
Patch

Review of attachment 8379120 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +375,5 @@
> +        // In some cases before-first-paint gets called before
> +        // RecvUpdateDimensions is called and therefore before we have an
> +        // mInnerSize value set. In such cases defer initializing the viewport
> +        // until we we get an inner size.
> +        if (mInnerSize.width && mInnerSize.height) {

It might enhance readability to introduce a HaveValidInnerSize() function and use it or its negation as appropriate.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Updated to pull out a HasValidInnerSize function as per review comments. Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=ba1bed82e0a3; if that's green I'll land the patch.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f019a781ef44

Comment 29

[Phil Ringnalda (:philor)]

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

Comment 30

[Joe Cheng [:jcheng] (please needinfo)]

move to 1.3T? triage to decide if this is wanted for tarako
if not, let's move this back to 1.5?

Comment 31

[[:fabrice] Fabrice Desré]

No we don't want that on 1.3t.

Comment 32

[William Hsu [:whsu]]

Verified it. Cannot reproduce this bug.
Also, no failure shows on keyboard testing result (Gaia UI test).
Thanks.

* Build Information:
 - Gaia      f55fc5c507312c7aac51ec9bb73061fd4ed5c5fb
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/3285e030d9c0
 - BuildID   20140504160202
 - Version   32.0a1