941468 - Authorizing a Google calendar account doesn't display keyboard to enter details

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[cajbir (:cajbir)]

When adding a Google Calendar the OAuth browser page requesting login details doesn't display the keyboard. This means you can't type in the username or password and so can't add the calendar.

Steps to reproduce:

1) Start Calendar
2) Press settings in top left
3) Press '+' to add a calendar
4) Choose 'Google'
5) Press in the 'Email' field in the authorization page

Expected results:

6) Keyboard appears to type email

Actual results:

6) No keyboard appears

Tested on a ZTE open running 'master' branch. Git commit gaia  298e6c5af4fe0cc0c4cfd88f7973bdd08e33aa51 and gecko 017ed20347cae2cd892d7ef144e7b36cd44f0185.

Comment 1

[Jason Smith [:jsmith]]

That sounds like a very recent regression.

Comment 2

[J Zimbrick]

Regression Window:

Last Working Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20131118040206
Gaia: 28e7dd59cf5b8ad9b197c59fbf92603e934e0474
Gecko: d58ab6f6ca0a
Version: 28.0a1
Base Image: V1.2_20131115

First Broken Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20131118094134
Gaia: 5d2ce56dbfbba702a487cd17efd19cdd62b2ff57
Gecko: f2adb62d07eb
Version: 28.0a1
Base Image: V1.2_20131115

Comment 4

[James Lal [:lightsofapollo]]

isn't this fallout from the OOP keyboard stuff? Nothing in calendar has changed here

Comment 5

[Natalya Kot [:nkot]]

Attachment: logcat

adding logcat from dup

Comment 7

[Ben Francis [:benfrancis]]

I can reproduce this on a recent 1.2 build on a keon from Geeksphone and the current master on a hamachi, nominating for koi+ just in case it's reproduceable on 1.2 on other devices.

It doesn't reproduce on the Facebook authentication popup in the contacts app so it doesn't seem to be a general keyboard issue.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Ben Francis [:benfrancis] from comment #7)
> I can reproduce this on a recent 1.2 build on a keon from Geeksphone and the
> current master on a hamachi, nominating for koi+ just in case it's
> reproduceable on 1.2 on other devices.
> 
> It doesn't reproduce on the Facebook authentication popup in the contacts
> app so it doesn't seem to be a general keyboard issue.

There's already a window present here on comment 2.

Comment 9

[Jason Smith [:jsmith]]

There's no commits in the regression range for Gaia in the keyboard app. This seems like a gecko bug.

Leaving QA Wanted to confirm this reproduces on a production device on 1.2.

Comment 10

[Natalya Kot [:nkot]]

(In reply to Jason Smith [:jsmith] from comment #9)
> 
> Leaving QA Wanted to confirm this reproduces on a production device on 1.2.

a dup bug 942773 was filed against 1.2, Buri device, so yes, it does repro

Comment 11

[Natalya Kot [:nkot]]

(In reply to Natalya Kot [:nkot] from comment #10)
> (In reply to Jason Smith [:jsmith] from comment #9)
> > 
> > Leaving QA Wanted to confirm this reproduces on a production device on 1.2.
> 
> a dup bug 942773 was filed against 1.2, Buri device, so yes, it does repro

sorry, i meant dup bug 942232

Comment 12

[Jason Smith [:jsmith]]

Suspected patch to cause this is bug 931746.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d58ab6f6ca0a&tochange=f2adb62d07eb

Comment 13

[brouberol]

I experience the same bug on a recent build.

Phone: ZTE Open
Version ID: 20131130151708
FFOS version: 1.2.0.0-prerelease
Git commit: 075e60c878b0eca68fba9e00bc85cb6eac03578a

Comment 14

[Preeti Raghunath(:Preeti)]

Dylan

Please have someone look at it as it seems to be a calendar issue.

Comment 15

[Dylan Oliver [:doliver]]

(In reply to Jason Smith [:jsmith] from comment #12)
> Suspected patch to cause this is bug 931746.
> 
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=d58ab6f6ca0a&tochange=f2adb62d07eb

Kanru, is this a known regression? Do you need assistance from someone familiar with Calendar to help research it?

Comment 16

[James Lal [:lightsofapollo]]

Gareth is on PTO this week so I will take a look. The root cause is very unlikely calendar (nothing here has changed since 1.0.1). I will report back tomorrow.

Comment 17

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

Yes, it's a regression from bug 931746. I'll take a look.

Comment 18

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

The OAuth2 page has an in-process browser frame in a remote app frame. The remote app frame message manager MMc1_1 is connected to the chrome window (Keyboard.jsm) via parent frame message manager MMp1_1. MMp1_1 is registered to Keyboard.jsm when we created the remote app frame. However the in-process browser frame has frame message manager MMp1_1_1 and MMc1_1_1 which are not know to the Keyboard.jsm. So when we filter out the message from the parent frame in bug 931746, Keyboard.jsm couldn't receive any message from the in-process browser.

 Parent side                         Child side
-------------                       ------------
 global MMg (B2G)
  |
  +-->window MMw1 (Keyboard.jsm)
       |
       +-->frame MMp1_1<------------>frame MMc1_1 (Calendar)
                                       frame MMp1_1_1<--->frame MMc1_1_1 (OAuth2)

Comment 19

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

Attachment: Part 1. Make all browser-element message manager connected

Connect browser-element message managers so we could use the global manager. Part 2 will make the "remote in-process" mm also connect to its parent.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8342303 [details] [diff] [review]
Part 1. Make all browser-element message manager connected


>+    nsCOMPtr<nsIContent> frameElement = mOwnerContent;
This could be just nsIContent*


>+    while (frameElement) {
>+      nsPIDOMWindow* win = frameElement->OwnerDoc()->GetWindow();
>+      if (win) {
>+        frameElement = win->GetFrameElementInternal();
>+        if (!frameElement) {
>+          break;
>+        }
>+      }
>+      nsCOMPtr<nsIFrameLoaderOwner> loaderOwner = do_QueryInterface(frameElement);
>+      nsRefPtr<nsFrameLoader> frameLoader = loaderOwner->GetFrameLoader();
>+      if (frameLoader->OwnerIsBrowserOrAppFrame()) {
>+        parentManager = frameLoader->mMessageManager;
>+        break;
>+      }
>+    }
Hmm, the setup becomes a bit odd. leaf message manager gets children.
Also, special casing OwnerIsBrowserOrAppFrame feels wrong.

How is the message passing supposed to work in this setup?


Why can't events be used for in-process communication when need to deal with not-at-chrome-boundary message manager communication with global mm?


(I'm not saying no to this, but this feels quite risky, and I don't quite understand what kind of message passing we want in this case. And what kind of frame script loading?)

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #20)
> Hmm, the setup becomes a bit odd. leaf message manager gets children.

Maybe make all message managers the children of the chrome window?

> Also, special casing OwnerIsBrowserOrAppFrame feels wrong.

Agree.

> How is the message passing supposed to work in this setup?
> 
> 
> Why can't events be used for in-process communication when need to deal with
> not-at-chrome-boundary message manager communication with global mm?

Hmm, either way I don't know how to do bidirectional communication. In current setup the parent wmm needs to send message to the child. So the top message manager has to pass the message/event to the correct child. I could include message id in each message and response but that looks ugly...

  parent                    child
----------                ---------
  pwmm1 <-----------------> cwmm1
                             |
                             +--> pwmm2 <----> cwmm2

Maybe we could make pwmm2 created correctly at parent side...

  parent                    child
----------                ---------
  pwmm1 <-----------------> cwmm1
  pwmm2 <-----------------> cwmm2

Maybe simply make the second iframe remote could fix this bug.

But since bug 931746 isn't blocking, I'm going to back it out first.

Comment 22

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

Attachment: Only load forms.js once per-process

The top level forms.js is capable to handle all events from its descendents so load it only once here could avoid duplication and make in-process & oop use the same flow.

Comment 23

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8343649 [details] [diff] [review]
Only load forms.js once per-process

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

Looks good expect a few concerns as following.

::: dom/browser-element/BrowserElementChild.js
@@ +23,5 @@
>  docShell.setFullscreenAllowed(infos.fullscreenAllowed);
>  
>  
> +function parentDocShell(docshell) {
> +  let treeitem = docshell.QueryInterface(Ci.nsIDocShellTreeItem);

if docshell is null, we'll get an error here.

@@ +30,5 @@
> +
> +function isTopBrowserElement(docShell) {
> +  while (docShell) {
> +    docShell = parentDocShell(docShell);
> +    if (docShell && docShell.isBrowserOrApp) {

if docShell is null, should we return true here?

::: dom/inputmethod/Keyboard.jsm
@@ -88,5 @@
> -        mm.loadFrameScript(kFormsFrameScript, true);
> -      }
> -    } catch (e) {
> -      dump('Error loading ' + kFormsFrameScript + ' as frame script: ' + e + '\n');
> -    }

Please make sure this doesn't break gaia keyboard on Firefox Nightly.

::: dom/inputmethod/forms.js
@@ -345,5 @@
>            break;
>          }
>  
> -        // Only handle the event from our descendants
> -        if (target instanceof HTMLElement &&

You also need to remove the line of 
|let HTMLElement = Ci.nsIDOMHTMLElement|
at the beginning of this file.

Comment 24

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

(In reply to Yuan Xulei [:yxl] from comment #23)
> Comment on attachment 8343649 [details] [diff] [review]
> Only load forms.js once per-process
> 
> Review of attachment 8343649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good expect a few concerns as following.
> 
> ::: dom/browser-element/BrowserElementChild.js
> @@ +23,5 @@
> >  docShell.setFullscreenAllowed(infos.fullscreenAllowed);
> >  
> >  
> > +function parentDocShell(docshell) {
> > +  let treeitem = docshell.QueryInterface(Ci.nsIDocShellTreeItem);
> 
> if docshell is null, we'll get an error here.

There is always a current docShell so I think it's unlikely this utilitiy function will get a null docshell. I'll return null here.

> 
> @@ +30,5 @@
> > +
> > +function isTopBrowserElement(docShell) {
> > +  while (docShell) {
> > +    docShell = parentDocShell(docShell);
> > +    if (docShell && docShell.isBrowserOrApp) {
> 
> if docShell is null, should we return true here?

We return true right after the while loop.
 
> ::: dom/inputmethod/Keyboard.jsm
> @@ -88,5 @@
> > -        mm.loadFrameScript(kFormsFrameScript, true);
> > -      }
> > -    } catch (e) {
> > -      dump('Error loading ' + kFormsFrameScript + ' as frame script: ' + e + '\n');
> > -    }
> 
> Please make sure this doesn't break gaia keyboard on Firefox Nightly.

Yeah, it works great!
 
> ::: dom/inputmethod/forms.js
> @@ -345,5 @@
> >            break;
> >          }
> >  
> > -        // Only handle the event from our descendants
> > -        if (target instanceof HTMLElement &&
> 
> You also need to remove the line of 
> |let HTMLElement = Ci.nsIDOMHTMLElement|
> at the beginning of this file.

OK

Comment 25

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

Attachment: Only load forms.js once per-process v1.1

Comment 26

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8344161 [details] [diff] [review]
Only load forms.js once per-process v1.1

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

Kanru, thanks! r=me if the following comments is addressed :-)

::: dom/browser-element/BrowserElementChild.js
@@ +27,5 @@
> +  if (!docshell) {
> +    return null;
> +  }
> +  let treeitem = docshell.QueryInterface(Ci.nsIDocShellTreeItem);
> +  return treeitem.parent ? treeitem.parent.QueryInterface(Ci.nsIDocShell) : null;

This line indicates the parent doc shell may be null.

@@ +32,5 @@
> +}
> +
> +function isTopBrowserElement(docShell) {
> +  while (docShell) {
> +    docShell = parentDocShell(docShell);

I don't want to nitpick, but if we get null parent doc shell, the while loop goes infinite. Though it is unlikely to happen, we need guard against it.

Comment 27

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

(In reply to Yuan Xulei [:yxl] from comment #26)
> Comment on attachment 8344161 [details] [diff] [review]
> Only load forms.js once per-process v1.1
> 
> Review of attachment 8344161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Kanru, thanks! r=me if the following comments is addressed :-)
> 
> ::: dom/browser-element/BrowserElementChild.js
> @@ +27,5 @@
> > +  if (!docshell) {
> > +    return null;
> > +  }
> > +  let treeitem = docshell.QueryInterface(Ci.nsIDocShellTreeItem);
> > +  return treeitem.parent ? treeitem.parent.QueryInterface(Ci.nsIDocShell) : null;
> 
> This line indicates the parent doc shell may be null.

It is possible and we use it to check if this is the top docShell.
 
> @@ +32,5 @@
> > +}
> > +
> > +function isTopBrowserElement(docShell) {
> > +  while (docShell) {
> > +    docShell = parentDocShell(docShell);
> 
> I don't want to nitpick, but if we get null parent doc shell, the while loop
> goes infinite. Though it is unlikely to happen, we need guard against it.

I don't understand. The while loop should stop if docShell is null.

Comment 28

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Kan-Ru Chen [:kanru] from comment #27)
>  
> > @@ +32,5 @@
> > > +}
> > > +
> > > +function isTopBrowserElement(docShell) {
> > > +  while (docShell) {
> > > +    docShell = parentDocShell(docShell);
> > 
> > I don't want to nitpick, but if we get null parent doc shell, the while loop
> > goes infinite. Though it is unlikely to happen, we need guard against it.
> 
> I don't understand. The while loop should stop if docShell is null.
Sorry, I made a mistake. The loop won't go infinite :-)

Comment 29

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

https://hg.mozilla.org/integration/b2g-inbound/rev/3e9af8f5053c

Comment 30

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

Attachment: Patch for b2g26_v1_2

not tested yet

Comment 31

[Kai-Chih Hu [:khu]]

Because 1.2 is going to be finished, should we mark this bug from koi+ to 1.3+? Thanks.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e9af8f5053c

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/464f20278a37

Comment 34

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

Attachment: Updated patch for v1.2

tested.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Pushed as an interdiff with the previous patch.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4d1c05b3d6f1