1213811 - [TSF] Bug 1208043 can be reproduced with TavultesoftKeyman90 or TavultesoftKeyman80

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Marc Durdin]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

In contentEditable sections (such as Compose Message page in Yahoo Mail where this was first reported), the Enter and Backspace keys are misbehaving when a Tavultesoft Keyman 9 input method (www.keyman.com) is active in Windows.


Actual results:

The Enter key goes to the next line but as soon as any other character is typed, the cursor moves back to the previous position, and input continues there. Sometimes after this happens, but not always, Backspace deletes just one character and then the cursor disappears.


Expected results:

Pressing Enter should have inserted a new line, and then subsequent input should have been inserted on that line.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

What version are you using? If you're using 41, I guess that it's fixed in 43 (Aurora) since your report looks like same as bug 1208043. Could you check if your reported bug can be reproduced with 43 Aurora? (In 42, for the risk management, the patch is restricted only for MS Korean IME, though.)
https://www.mozilla.org/en-US/firefox/channel/#developer

Comment 2

[Marc Durdin]

Correct, I've just tested and this appears to be resolved in 43.0a2 (2015-10-13).

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you want to fix this bug on 42 too, we need to add the TIPs to the whitelist. However, if it's too many TIPs, we cannot do that in 42. How many TIPs do you have this bug?

Comment 4

[Marc Durdin]

It'd be great to get a patch in for 42 - I didn't realise that was still possible :) 

We have two TIPs that experience this issue; the CLSIDs are:

487EB753-DB93-48C5-9E6A-4398E777C61D (TavultesoftKeyman90)
7ba04432-8609-4fe6-bff7-971091de0933 (TavultesoftKeyman80)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[Tracking Requested - why for this release]:

Only Beta, the fix of bug 1208043 is enabled with whitelist. Currently, it only allows to run with MS Korean IME. However, we should allow it to TavultesoftKeyman90 and TavultesoftKeyman80 too.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9164d3de717a

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

I cannot promise we can take this into 42 Beta since it's going to be released on 11/3, but let's do the best what we can do.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

oops, fix the wrong comment.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8673679 [details] [diff] [review]
Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

This is beta only patch. Could you review this? After try build comes, I'll ask to check if this is actually fixed on the TIPs to the reporter.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you test with this build?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a216562c3f25/try-win32/firefox-42.0.en-US.win32.zip

If the IDs you specified isn't the GUID which is passed to ITfInputProcessorProfileActivationSink::OnActivated(), please tell me the correct GUID of the profile.
https://msdn.microsoft.com/ja-jp/library/windows/apps/aa381940.aspx

Comment 11

[Marc Durdin]

The IDs I specified are the rclsid parameter passed into OnActivated(). As Keyman includes hundreds of layouts, it generates a guidProfile at install time for each layout. You'll need to store mActiveTIPCLSID as well and compare against that?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Marc Durdin from comment #11)
> The IDs I specified are the rclsid parameter passed into OnActivated(). As
> Keyman includes hundreds of layouts, it generates a guidProfile at install
> time for each layout.

Hmm, I've worried about this situation. We've never had CLSID check code...

> You'll need to store mActiveTIPCLSID as well and
> compare against that?

Yes. Probably, right. However, I'm not 100% sure if it's safe. I.e., I'm not 100% sure if a CLSID of text service is never shared by two or more TIPs accidentally. I think that we cannot take such risk in Beta stage (i.e., if this would be necessary for 44, I could land such code). So, unfortunately, we cannot fix this bug in 42 except we'd find better solution or you know some documents which guarantee that same CLSID text service cannot be installed.

Until we find another solution, marking this as WONTFIX.

Comment 13

[Marc Durdin]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> Yes. Probably, right. However, I'm not 100% sure if it's safe. I.e., I'm not
> 100% sure if a CLSID of text service is never shared by two or more TIPs
> accidentally. I think that we cannot take such risk in Beta stage (i.e., if
> this would be necessary for 44, I could land such code). So, unfortunately,
> we cannot fix this bug in 42 except we'd find better solution or you know
> some documents which guarantee that same CLSID text service cannot be
> installed.

I'm not entirely sure what you are asking me, but, yes, the CLSID is unique to the Keyman TIP. Multiple instances of the Keyman TIP can and often are installed at once, each with its own unique guidProfile. I'm not sure why this would be a problem?

You'll never get two TIPs with different implementations with the same CLSID. Each CLSID is unique to the class implementing it: https://msdn.microsoft.com/en-us/library/windows/desktop/ms691424(v=vs.85).aspx Does that help answer your concern or have I got the wrong end of the stick?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Marc Durdin from comment #13)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> > Yes. Probably, right. However, I'm not 100% sure if it's safe. I.e., I'm not
> > 100% sure if a CLSID of text service is never shared by two or more TIPs
> > accidentally. I think that we cannot take such risk in Beta stage (i.e., if
> > this would be necessary for 44, I could land such code). So, unfortunately,
> > we cannot fix this bug in 42 except we'd find better solution or you know
> > some documents which guarantee that same CLSID text service cannot be
> > installed.
> 
> I'm not entirely sure what you are asking me, but, yes, the CLSID is unique
> to the Keyman TIP. Multiple instances of the Keyman TIP can and often are
> installed at once, each with its own unique guidProfile. I'm not sure why
> this would be a problem?

My question is, whether same CLSID is used by other text service too. Yes, I know that CLSID should be generated as a unique key. However, the important point is that, when another text service which has same CLSID of the other text service is being installed, whether the install is failed or not. Only if the former case, we can trust the value even in this timing (only remaining 2 weeks!).

> You'll never get two TIPs with different implementations with the same
> CLSID. Each CLSID is unique to the class implementing it:
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms691424(v=vs.85).
> aspx Does that help answer your concern or have I got the wrong end of the
> stick?

Therefore, I'm not 100% sure that whether adding CLSID check for checking active TIP is enough safe or not. That's the cause of that I'm afraid to land a patch checking CLSID. (If we have enough time, Nightly/Aurora/Beta testers can test it.)

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4277e9d5379

Comment 16

[Marc Durdin]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> (In reply to Marc Durdin from comment #13)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> 
> My question is, whether same CLSID is used by other text service too. Yes, I
> know that CLSID should be generated as a unique key. However, the important
> point is that, when another text service which has same CLSID of the other
> text service is being installed, whether the install is failed or not. Only
> if the former case, we can trust the value even in this timing (only
> remaining 2 weeks!).

You can have multiple TIPs with the same CLSID installed, but only if they use the same class object. For example, the TavultesoftKeyman90 TIP may be installed with a profile GUID for Vietnamese and a second profile GUID for Khmer. These two TIPs will have the same CLSID but different profile GUIDs.

A TIP is implemented by a DLL. The CLSID ties the TIP to the DLL and is stored in the registry in HKCR\CLSID. It is not possible to install two TIPs with different DLL implementations but the same CLSID: one would overwrite the other.

Furthermore, with the global uniques of CLSIDs, it is mathematically improbable for two different DLLs to accidentally use the same CLSID.  http://stackoverflow.com/a/2977648/1836776

> Therefore, I'm not 100% sure that whether adding CLSID check for checking
> active TIP is enough safe or not. That's the cause of that I'm afraid to
> land a patch checking CLSID. (If we have enough time, Nightly/Aurora/Beta
> testers can test it.)

It is as safe as testing the profile GUID. The CLSID is registered when the TIP is installed, and directly ties the TIP to the DLL that implements it. The CLSID is typically hard-coded into the DLL.

Another example (not for implementation, just for reference): Microsoft have a TIP that has a single CLSID and multiple GUIDs as well: the Table Text Service. Its CLSID is {E429B25A-E5D3-4D1F-9BE3-0C608477E3A1}, and some of its profile GUIDs are, for example: {8F96574E-C86C-4bd6-9666-3F7327D4CBE8} for Amharic, or {409C8376-007B-4357-AE8E-26316EE3FB0D} for Yi.

Hope this helps.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds like enough safe to me. Could you test this build with your TIPs?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5963ef4023c4/try-win32/firefox-42.0.en-US.win32.zip

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

According to the above comment, checking CLSID of text service is enough safe for beta. If you don't think so, please -'ing this.

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 8674197 [details] [diff] [review]
Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

r=me assuming that this patch fixes the problem.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Kimura-san.

I'll request approval for beta after Marc confirms the fix on the test build.

Comment 21

[Marc Durdin]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> Sounds like enough safe to me. Could you test this build with your TIPs?
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.
> com-5963ef4023c4/try-win32/firefox-42.0.en-US.win32.zip

Yes, thank you Nakano-san. In this build, our TIP works correctly in contentEditable elements and elsewhere.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8674197 [details] [diff] [review]
Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

Approval Request Comment
[Feature/regressing bug #]: Regressed by enabling TSF in 41, and Beta has a fix for this which is restricted to only MS Korean IME (bug 1208043).
[User impact if declined]: Users of TavultesoftKeyman which supports a lot of languages (http://keyman.com/) cannot type text as they expected in some rich text editor which has <p> elements.
[Describe test coverage new/current, TreeHerder]: Marc Durdin who must work at Tavltesoft tested the patched build with the IMEs.
[Risks and why]: The risk is much low since this is just adding the IMEs into the whitelist of bug 1208043.
[String/UUID change made/needed]: Nothing.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8674197 [details] [diff] [review]
Include TavultesoftKeyman 90 and 80 to the whitelist of the fix of bug 1208043

OK, should not add risks to the release. Should be in 42 beta 8.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5bd26c10432

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This was 42 specific bug, so, that was already fixed.