1444571 - [GTK][IIIMF] Make IMContextWrapper::PrepareToDestroyContext() GTK3-aware

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

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

We have a hack to avoid crashing with IIIMF which is really old IM, but still used by some ATOK X (Japanese IME) users:
https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/widget/gtk/IMContextWrapper.cpp#414-449

However, this hack wasn't implemented when porting to GTK3 and without this TODO fix, our widget has been changed from GTK2 to GTK3. After fixing bug 1443421, we can detect which IM is used in the session.  So, let's reimplement this hack.

Comment 1

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

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

Comment 2

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

Piro-san:

Could you test this try build?
https://queue.taskcluster.net/v1/task/IoF4HBrsS5e-ZvoDFTLY-A/runs/0/artifacts/public/build/target.tar.bz2

Run it on terminal with |MOZ_LOG=nsGtkIMModuleWidgets:5,sync|, then, you'll see a line including "PrepareToDestroyContext()" when you close a window (it's okay to launch and close the window). If my patch fails to prevent the crash, it always crashes (unless if my patch failed to detect active IM is IIIMF). So, closing a window without crash must work perfectly.

Comment 3

[YUKI "Piro" Hiroshi]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> Run it on terminal with |MOZ_LOG=nsGtkIMModuleWidgets:5,sync|, then, you'll
> see a line including "PrepareToDestroyContext()" when you close a window
> (it's okay to launch and close the window). If my patch fails to prevent the
> crash, it always crashes (unless if my patch failed to detect active IM is
> IIIMF). So, closing a window without crash must work perfectly.

I've confirmed that the build reports the log message "PrepareToDestroyContext()" and Ctrl-Q doesn't report any crash. Steps:

 1. Start Nightly.
 2. Hit Ctrl-N multiple times to open multiple windows.
 3. Hit Ctrl-L and start input via IIIMF.
 4. Hit Ctrl-Q to exit.

Comment 4

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

Thank you very much! That means my patch succeed to port the hack to GTK3 without dangerous approach!

Comment 5

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

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

Comment 6

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

Attachment: Bug 1444571 - Prevent IIIM module from being unloaded with grabbing GtkIMContextIIIM class with static variable

IIIMF is really old IME framework.  In these days, it's not used as default IM
module of any major distributions.  However, ATOK X which is old proprietary
IME requires IIIMF and it's still used by some Japanese IME users.  Therefore,
we need to take back the hack to prevent crash caused by IIIMF.

We did increment refcount of GtkIMContextIIIM class to prevent IIIM module
from being unloaded.  However, it was not ported when we changed default
toolkit from GTK2 to GTK3.  So, this is doing the porting.

Unfortunately, the instance of GtkIMContextIIIM is wrapped by opacity struct.
So, it's not safe to access the pointer with declaring a mimic struct.
Therefore, we should directly get GType from the name with calling
g_type_from_name("GtkIMContextIIIM") instead of using G_TYPE_FROM_INSTANCE()
and g_type_name().

Review commit: https://reviewboard.mozilla.org/r/227306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227306/

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8958717 [details]
Bug 1444571 - Prevent IIIM module from being unloaded with grabbing GtkIMContextIIIM class with static variable

https://reviewboard.mozilla.org/r/227306/#review233714

The approach looks good, thanks.  Can you touch up the comment and variable name, please, to clarify that the reference is to the class (see below).

::: widget/gtk/IMContextWrapper.cpp:610
(Diff revision 1)
> +            // by GTK3.  Perhaps, it must be enough to grab our dummy context
> +            // to prevent the IIIM module from being unloaded.
> +            GType IIMContextType = g_type_from_name("GtkIMContextIIIM");
> +            if (IIMContextType) {
> +                sMasterContextOfIIIMContext = g_type_class_ref(IIMContextType);

The type name "GtkIMContextIIIM" is the same as was used for the slave in GTK2, and so I assume that is still the slave class.  I don't know what the "master" or "dummy" context is though.

The static variable is still a reference to the class rather than an instance and so I would keep the |gtk_iiim_context_class| name.  (It is not a reference to a GtkIMContext instance.)

::: widget/gtk/IMContextWrapper.cpp:626
(Diff revision 1)
>          // Mute unused variable warning:
> -        (void)gtk_iiim_context_class;
> +        Unused << sMasterContextOfIIIMContext;

I expect this is no longer required now that there is an !sMasterContextOfIIIMContext test.

Comment 8

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

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

Comment 9

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

Comment on attachment 8958717 [details]
Bug 1444571 - Prevent IIIM module from being unloaded with grabbing GtkIMContextIIIM class with static variable

https://reviewboard.mozilla.org/r/227306/#review233714

> The type name "GtkIMContextIIIM" is the same as was used for the slave in GTK2, and so I assume that is still the slave class.  I don't know what the "master" or "dummy" context is though.
> 
> The static variable is still a reference to the class rather than an instance and so I would keep the |gtk_iiim_context_class| name.  (It is not a reference to a GtkIMContext instance.)

Ah, sorry. The comment explains older approach when I was looking for a better way.

I'll rewrite all comments to explain what the code does (i.e., grabbing the class, not instance), and the reason.

But I rename the static variable to sGtkIIIMContextClass to confirm to our coding style.

> I expect this is no longer required now that there is an !sMasterContextOfIIIMContext test.

Indeed.

Comment 10

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

Comment on attachment 8958717 [details]
Bug 1444571 - Prevent IIIM module from being unloaded with grabbing GtkIMContextIIIM class with static variable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227306/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68dfe5ee5b80
Prevent IIIM module from being unloaded with grabbing GtkIMContextIIIM class with static variable r=karlt

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/68dfe5ee5b80