Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 6 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 6 years ago

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.

	YUKI "Piro" Hiroshi
	Comment 3 • 6 years ago

(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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 6 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 6 years ago

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

	Karl Tomlinson (:karlt)
	Comment 7 • 6 years ago
mozreview-review

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 8 • 6 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 6 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 11 • 6 years ago

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/68dfe5ee5b80
Prevent IIIM module from being unloaded with grabbing GtkIMContextIIIM class with static variable r=karlt

	Stefan Hindli [:stefan_hindli]
	Comment 12 • 6 years ago
bugherder

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