Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #847763 +++

This is hopefully the last bug of 3rd-party keyboard support. Once bug 847763 landed, we should not allow any keyboard app to run in-process. Doing so will break our security model.

I have patch available.

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 2 • 11 years ago

Comment on attachment 8335150 [details]
mozilla-b2g:master PR#13862

Rudy, could you review my patch? Thanks!

	Jason Smith [:jsmith]
	Comment 3 • 11 years ago

This might have too much risk at this point in the game to take for 1.2 - OOP comes with quite a lot of regression risk. None of our test runs have ever flipped this pref, so I'm worried about regression fallout here.

Putting this into koi? for discussion.

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 4 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #3)
> This might have too much risk at this point in the game to take for 1.2 -
> OOP comes with quite a lot of regression risk. None of our test runs have
> ever flipped this pref, so I'm worried about regression fallout here.
> 
> Putting this into koi? for discussion.

Enabling out-of-process has been tested locally by developers for months. It's unlikely it would cause that much regression (as we have seen for many APIs during v1.0.1).

At the end of the day we would either have to (1) enable oop for real or (2) disallowing 3rd-party keyboard app being installed, to ship a secure product. It's either (1) or (2). So this bug should stay koi+'d so we know what needs to be fixed, i.e. blocking release.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 5 • 11 years ago

Comment on attachment 8335150 [details]
mozilla-b2g:master PR#13862

r=me.

Tim, thanks for handling this.

	Jason Smith [:jsmith]
	Comment 6 • 11 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #4)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > This might have too much risk at this point in the game to take for 1.2 -
> > OOP comes with quite a lot of regression risk. None of our test runs have
> > ever flipped this pref, so I'm worried about regression fallout here.
> > 
> > Putting this into koi? for discussion.
> 
> Enabling out-of-process has been tested locally by developers for months.
> It's unlikely it would cause that much regression (as we have seen for many
> APIs during v1.0.1).

That's not good enough. That means no one has looked it formal testing.

> 
> At the end of the day we would either have to (1) enable oop for real or (2)
> disallowing 3rd-party keyboard app being installed, to ship a secure
> product. It's either (1) or (2). So this bug should stay koi+'d so we know
> what needs to be fixed, i.e. blocking release.

This is a rel man call at this point, which needs to be discussed in triage. Flipping the flag back.

	Jason Smith [:jsmith]
	Comment 7 • 11 years ago

We've setup a 6pm call to talk about this in detail.

	[:fabrice] Fabrice Desré
	Comment 8 • 11 years ago

I'm not sure I'll be able to make the call, so here's my take on this:

The good:
- I don't expect functional regression in the keyboard API itself. This code was written with e10s in mind since day 1 (or 2).

The bad:
- We know that the keyboard is not the most responsive part of our UI in terms of delay from a tap on the vkb to a character appearing in the focused input field. Unfortunately I fear that this will be a bit worse with OOP keyboards (currently we have some "fake IPC" that does parent -> parent instead of child -> parent and is faster).
- One more process competing for resources while we're already tight on memory. We make sure to kill the keyboard and not the app using the keyboard but that's still not great. We are good on 512MB devices, a lot less on 256MB.

The ugly:
- No testing/dogfooding at all. We know how bad that went so far for 1.2.

My take is absolutely no for 1.2, and conditionally on 1.3 if we can turn on nuwa processes soon enough. In any case I also want to see some eiderticker performance measures to check that we don't regress reactivity too much.

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 9 • 11 years ago

Thanks Fabrice for the detailed analysis for what we are right now.

I am continuing discussion here (rather than rel-driver) because we are taking about technical risks.

1) Functional regressions:

Yes, there shouldn't be any InputMethod/Keyboard API functional regression, as Fabrice states. Neither mozborwser ignoreuserfocus (Bug 847763) as far as I could tell.

2) Performance regressions and other surprises:

I have not considered possible performance regression, which is what Fabrice pointed out. He have also pointed out the risk w/o having QA thoroughly testing the feature and dogfood, which are all valid arguments.

3) The way move forward:

 - A) Ship as-is without uplift either bug 847763 nor attachment 8335150 [details]. This is not an acceptable solution because it would allow 3rd-party keyboard to run in-process, thus breaking our security model.
 - B) Uplift both bug 847763 and attachment 8335150 [details], enable oop keyboard for everyone. This is not acceptable because of the risks.
 - C) Uplift bug 847763 but kept built-in keyboard app stay in-process by default. Write another Gaia patch enable oop keyboard as a developer option. Only when developer option is enabled, we will allow 3rd-party keyboard apps to launch. I think this unlocks future developments (both FxOS and external keyboard vendors), and also minimize the risk.
 - D) Not uplifting bug 847763. Without bug 847763 keyboard will not be useable if being run oop'd, so there is no point to keep a developer option. Since there is no way to run keyboard apps oop, we would need a Gaia patch to reject any keyboard apps other than built-in keyboard app.

I would personally argue for (C). Patch for (C) would be < 30 lines of code and will be less than patch for (D). (C) also provide a way forward further in v1.3 any beyond. If we check-in patch for (D) I would like to know hear what can we do next.

I can come up with patch for (C) in the next few hours.

If we are having a meeting about this please send the invite ahead of sleeping hours of my timezone. Thank you.

	Jason Smith [:jsmith]
	Comment 10 • 11 years ago

Option C sounds reasonable. It keeps the OOP regression risk under control for normal users while still establishing a developer facing release of keyboard apps for developers who want to start building keyboard apps for Firefox OS. Followup questions based off of this then are:

1. So that means we should set bug 847763 back to a koi+, right?
2. Should we file a separate bug for enabling OOP keyboard as a developer option?
3. This bug itself will no longer be a blocker for 1.2, as it's enabling OOP support for keyboard apps across the board, right?
4. Is there anything I'm missing here from the above 3 questions on action items thatwe need to do?

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 11 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #10)
> Option C sounds reasonable. It keeps the OOP regression risk under control
> for normal users while still establishing a developer facing release of
> keyboard apps for developers who want to start building keyboard apps for
> Firefox OS. Followup questions based off of this then are:
> 
> 1. So that means we should set bug 847763 back to a koi+, right?

Yes.

> 2. Should we file a separate bug for enabling OOP keyboard as a developer
> option?

Yes, unless we want to reuse this bug and change the summary.

> 3. This bug itself will no longer be a blocker for 1.2, as it's enabling OOP
> support for keyboard apps across the board, right?

Yes, unless we want to reuse this bug and change the summary.

> 4. Is there anything I'm missing here from the above 3 questions on action
> items that we need to do?

I don't think we miss anything (other than I need to come up with a patch for option (C))

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 13 • 11 years ago

Comment on attachment 8335936 [details]
mozilla-b2g:master PR#13898

This is my option (C) patch.

I realized disallowing 3rd-party keyboard to be installed is not a proper fix because people could enable keyboard oop and install *then* turn it off. So for this patch, I simply tweak the keyboard_manager.js on places where it actually create the frame, and ensure a 3rd-party app is always run oop'd and our own app stay in-process unless user flip the developer option.

This can keep our phone secure while keeping development unblocked.

If we want to do what I said earlier on disallowing 3rd-party keyboard to be installed while keyboard oop option is turned off, the patch would have to touch a wider area and thus increase the risk.

	Jason Smith [:jsmith]
	Comment 14 • 11 years ago

For the sake of avoiding confusion with morphing bugs, I've filed bug 941627 to track option C's implementation.

Moving this to a non-blocker since we're going to go with option C's less riskier solution instead of turning OOP across the board for 1.2.

	Tim Guan-tin Chien [:timdream] (please needinfo) Reporter
	Comment 15 • 11 years ago

Let's revisit this bug when keyboard oop is actually ready in future versions.