Closed Bug 823201 Opened 7 years ago Closed 7 years ago

Keyboard doesn't work in OOP apps

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

Attachments

(1 file, 1 obsolete file)

In the current gecko mc build I can't add a contact or send a sms because the keyboard isn't working. I can type, the keys get highlighted and I see word suggestions but nothing happens in the actual text field.

This blocks all the removing PhonenumberJS from gaia and add PhonenumberJS to gecko work.
blocking-basecamp: --- → ?
We need to back out whatever regressed this.
maybe bug 806540?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Actually, are you testing with a build made from after
> https://hg.mozilla.org/mozilla-central/rev/67900b58b4f8 ?

Yes. My current tip is mc:
changeset:   116438:1b6ab3a080d8
tag:         tip
user:        Jonathan Kew <jkew@mozilla.com>
date:        Wed Dec 19 09:42:25 2012 +0000
summary:     bug 821442 - eliminate the unreliable mFamily back-pointer in gfxFontEntry, and instead pass/track font family explicitly where needed. r=roc
That's an IPC related bug. If you turn off OOP in the settings, the keyboard works.
good revision: 548d2c909b81
bad revision:
https://tbpl.mozilla.org/?rev=ba26dc1c6267

So it happened between 12/15 and 12/17.
And the winner is bug 813445
Blocks: 813445
Severity: major → blocker
Summary: Keyboard doesn't work in the contacts and sms app → Keyboard doesn't work in OOP apps
masayuki, could you look at this, or should we backout bug 813445?

And someone, please add tests for b2g.
And it only happens on the device. It works fine within the b2g desktop build. Maybe something in the gonk code?
(In reply to Gregor Wagner [:gwagner] from comment #10)
> And it only happens on the device. It works fine within the b2g desktop
> build. Maybe something in the gonk code?

Even with OOP turned on in the desktop build? (it's off by default now)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(In reply to Fabrice Desré [:fabrice] from comment #11)
> (In reply to Gregor Wagner [:gwagner] from comment #10)
> > And it only happens on the device. It works fine within the b2g desktop
> > build. Maybe something in the gonk code?
> 
> Even with OOP turned on in the desktop build? (it's off by default now)

Ok I didn't know that. If I use pref("dom.ipc.tabs.disabled", false); I can't even open an app or swipe.
http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#28

Should aResult be &aResult? My computer is busy now. Can somebody test it?
(In reply to Gregor Wagner [:gwagner] from comment #7)
> And the winner is bug 813445

Did you get a particular cset that regressed?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #14)
> (In reply to Gregor Wagner [:gwagner] from comment #7)
> > And the winner is bug 813445
> 
> Did you get a particular cset that regressed?

No. Will do it after testing the suggestion from comment 13.
It's OK, I'm doing a bisect right now for a separate issue that should find the same commit.
The code mentioned in comment 13 is the problem. Using &aResult doesn't make the compiler happy but a small rewrite seems to fix the problem.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#28
> 
> Should aResult be &aResult? My computer is busy now. Can somebody test it?

This deserialization code is just wrong.  It should be

     const paramType* buffer;
     return aMsg->ReadBytes(aIter, reinterpret_cast<const char**>(&buffer),
                            sizeof(paramType));
     *aResult = buffer;

ReadBytes() returns a pointer to the internal message buffer.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)

Sorry, this should be

  const paramType* buffer;
  if (!aMsg->ReadBytes(aIter, reinterpret_cast<const char**>(&buffer),
                       sizeof(paramType))) {
    return false;
  }
  *aResult = buffer;
  return true;
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → anygregor
Attachment #694195 - Flags: review?(jones.chris.g)
Attached patch patchSplinter Review
Attachment #694195 - Attachment is obsolete: true
Attachment #694195 - Flags: review?(jones.chris.g)
Attachment #694202 - Flags: review?(jones.chris.g)
Comment on attachment 694202 [details] [diff] [review]
patch

>diff --git a/widget/nsGUIEventIPC.h b/widget/nsGUIEventIPC.h

>+    if (!aMsg->ReadBytes(aIter, &outp, sizeof(*aResult))) {
>+      return false;
>+    }
>+

Nit: remove newline.

We need to land asap to fix regression, but jeez we need a test here.  I can't believe we don't have any already.  Please leave open for that.
Attachment #694202 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7f3548011c

leave open for tests
Whiteboard: leave-open
Duplicate of this bug: 822088
blocking-basecamp: ? → +
Component: Gaia::Keyboard → General
QA Contact: wachen
Doesn't nobody test the patched build yet?
Does this need to be uplifted to b2g18 and aurora?
blocking-basecamp: + → ---
Severity: blocker → normal
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: leave-open
You need to log in before you can comment on or make changes to this bug.