Closed Bug 1189125 Opened 6 years ago Closed 6 years ago

Allow xpcshell to use GTK

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
I've been writing some xpcshell tests for bug 1175770. That bug uses Services.appShell.createWindowlessBrowser to render web content in a headless frame. I ran into a problem where some of the HTML rendering code calls into GTK, which isn't initialized in xpcshell. I tried putting checks in front of all that code so that we return if we don't have a display to use. But there were just too many (I stopped after patching 5 functions).

I'm open to better ideas for this, but the easiest way seems to be to initialize GTK in xpcshell. Sorry for all the reviewers, but I feel like this could use input from multiple people.

One cool thing about this patch is that it allows us to use XPCShell as a kind of headless Gecko. I think we could use the drawWindow API to output the data to a file or network channel or something.

Also, I checked that other platforms don't have similar problems. It appears they don't (based on try).
Attachment #8640813 - Flags: review?(mh+mozilla)
Attachment #8640813 - Flags: review?(karlt)
Attachment #8640813 - Flags: review?(bobbyholley)
Comment on attachment 8640813 [details] [diff] [review]
patch

Review of attachment 8640813 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to glandium on this one.

::: js/xpconnect/shell/xpcshell.cpp
@@ +9,5 @@
>  #include <stdio.h>
>  
> +#ifdef MOZ_WIDGET_GTK
> +#include <gtk/gtk.h>
> +#endif

Add some comments here as to why we're doing this.
Attachment #8640813 - Flags: review?(bobbyholley)
Comment on attachment 8640813 [details] [diff] [review]
patch

Review of attachment 8640813 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/shell/xpcshell.cpp
@@ +33,5 @@
>  main(int argc, char** argv, char** envp)
>  {
> +#ifdef MOZ_WIDGET_GTK
> +    // sigh
> +    gtk_init(nullptr, nullptr);

I understand the appeal for a quick and dirty solution, but I wonder why we don't move Gtk initialization outside XRE_main to somewhere in widget/gtk. Karl, do you have an opinion?
Comment on attachment 8640813 [details] [diff] [review]
patch

I've seen a few issues re xpcshell things expecting GTK to be initialized, so
I suspect you are probably right that initializing GTK is easier than coping
with lack of initialization.

Is an X11 display connection required?  If not, then gtk_parse_args may be
sufficient instead of gtk_init, and more efficient if there are many or short
lived xpcshell processes.

I'm not familiar with all the uses of xpcshell so I think someone else should
decide weather it is appropriate to do this for all instances.  gtk_parse_args
should be mostly harmless, but opening an X11 connection is more significant.

If xpcshell processes are doing memory leak testing, and gtk_init is
necessary, then closing the GDK display as in nsAppRunner would be helpful,
but IIRC this is disabled for a bug in the GTK+3 version on our test machines.

I don't see g_thread_init() currently called for xpcshell.  If it is called,
then it would need to happen before gtk_init().

(In reply to Mike Hommey [:glandium] from comment #2)
> I understand the appeal for a quick and dirty solution, but I wonder why we
> don't move Gtk initialization outside XRE_main to somewhere in widget/gtk.
> Karl, do you have an opinion?

If it is only a gtk_init call, then I don't see much to gain from doing that
elsewhere, but having common init becomes useful if more is required.

If an X11 connection is necessary, but we don't want all xpcshell instances to
be X11 clients, then I guess it might be an option to initialize the
connection explicitly somewhere else in a way that only those that need it
will get it.  gfxPlatformGtk and maybe nsWidgetFactor (on nsWindow creation
perhaps) may be able to do this on demand, but I don't know how easily that
will work out with other possible entry points.  Perhaps each xpcshell
consumer can choose to buy in by loading a service, command line option or
something.
Attachment #8640813 - Flags: review?(karlt) → feedback+
FWIW, a long time ago, I made it possible to run xpcshell tests without a X server. That still mostly works, except for a few tests that end up needing one for whatever reason. I haven't looked why in a long while.

Now, with that being said, coming back to the motivation behind this bug, is an xpcshell test actually the right thing to use for tests in bug 1175770? Or said otherwise, why not make it a mochitest? Sorry if the reason is given in that bug, but I didn't find one with a cursory <ctrl>+f test.
Note that gabor, who originaly introduced createWindowLessBrowser opened bug 929898
right after creating this helper function.
This function was mainly introduced in the browser for addons, in order to stop relying on the hidden window. But it was also becoming handy for tests requiring a window/document.

(In reply to Mike Hommey [:glandium] from comment #4)
> Now, with that being said, coming back to the motivation behind this bug, is
> an xpcshell test actually the right thing to use for tests in bug 1175770?
> Or said otherwise, why not make it a mochitest?

I think Bill isn't the first one who want to write a xpcshell test involving a document...
I did and many devs around me in jetpack/devtools also wanted to do such thing.
Why? Because running a xpcshell test is much faster, a bit simplier to write and with a simplier/cleaner environment (there is no pollution from Firefox UI codebase. sometimes your test conflicts with existing components from firefox).
If we have that, I think we could replace most mochitest-chrome with simplier xpcshell tests.
But I can't think of a reason that would justify that WE DO HAVE TO DO THIS, we can keep writing mochitests as soon as we require a document. It would just be convenient.
Given that ContentChild.cpp immediately initializes GTK, and that there are many xpcshell tests that spawn content processes, I think the ship has sailed on running xpcshell tests without X. I just tried running a random test in network/test/unit_ipc without DISPLAY set and it failed.

So I don't think we're really losing any ground by doing this.
Also, the reasons for using xpcshell tests for bug 1175770 are exactly what Alexandre said: it's much nicer to use an xpcshell test if you can. A mochitest would work too.
Comment on attachment 8640813 [details] [diff] [review]
patch

Review of attachment 8640813 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/shell/xpcshell.cpp
@@ +32,5 @@
>  int
>  main(int argc, char** argv, char** envp)
>  {
> +#ifdef MOZ_WIDGET_GTK
> +    // sigh

This kind of comment is not useful in the tree.

@@ +33,5 @@
>  main(int argc, char** argv, char** envp)
>  {
> +#ifdef MOZ_WIDGET_GTK
> +    // sigh
> +    gtk_init(nullptr, nullptr);

You should pass *argc, *argv.
Attachment #8640813 - Flags: review?(mh+mozilla) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
This patch initializes GTK lazily from the gfxPlatformGtk constructor. That seems to work in the cases I've tried. It gets hit the first time we create a PuppetWidget, which is quite early in the process of creating the headless browser.

I looked at the GTK code and verified two things:
1. Whoever sets the cmd line args first wins. So calling gtk_parse_args and then gtk_init later is fine.
2. Calling gtk_init multiple times also appears fine. The patch tries to avoid this, but just in case you're worried about it :-). I didn't change some gtk_init callers in the plugin code and webapprt. I'm guessing they don't use gfxPlatformGtk, so that should be orthogonal to this change. But even if they do, it should still work.
Attachment #8640813 - Attachment is obsolete: true
Attachment #8641375 - Flags: review?(mh+mozilla)
Attachment #8641375 - Flags: review?(karlt)
Comment on attachment 8641375 [details] [diff] [review]
patch v2

Review of attachment 8641375 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsAppRunner.cpp
@@ +3707,5 @@
>    // opens.
>    if (!gtk_parse_args(&gArgc, &gArgv))
>      return 1;
> +
> +  gfxPlatformGtk::SetInitialized();

Considering the comment above, I'm afraid calling git_init here might be a cause of problems.
Attachment #8641375 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #10)
> Considering the comment above, I'm afraid calling git_init here might be a
> cause of problems.

SetInitialized does not call gtk_init. It just pretends that GTK has been initialized. Open to better names or a better way to do it.
Huh, I don't know why I read EnsureInitialized. Let's put that on exhaustion. This is better reviewed by Karl anyways.
Comment on attachment 8641375 [details] [diff] [review]
patch v2

This kind of thing looks good given gfxPlatformGtk initialization is good
enough to trigger opening the display for your use case.

As you say, it is safe to gtk_init() more than once, so drop sGtkInitialized
and things are much simpler.

>+    static void EnsureInitialized(int* argc = nullptr, char*** argv = nullptr);
>+    static void SetInitialized();

SetInitialized can then be removed.

gfxPlatformGtk::EnsureInitialized() doesn't ensure that gfxPlatformGtk is
initialized, and the args are never non null, and without sGtkInitialized it
need only call gtk_init(nullptr, nullptr), so remove the function as it just
becomes additional API to do the same thing.

ContentChild.cpp can probably call gfxPlatform::GetPlatform() if you prefer
that over gtk_init().

>+    // GTK may or may not get initialized in xpcshell, depending on whether we
>+    // need it. Either way, we set the command line args, which is a fairly
>+    // cheap operation.
>+    gtk_parse_args(&argc, &argv);

I'm a bit confused about "in xpcshell" because I think of this function/file
as xpcshell.  Also gtk_parse_args does init GTK; it just doesn't set the
default display.  So the comment should talk about the default display rather
than gtk initialization.  Perhaps "A default display may or may not be
required, and so is not created here, but we set the command line args, which
is a fairly cheap operation."
Attachment #8641375 - Flags: review?(karlt)
Attachment #8641375 - Flags: review-
Attachment #8641375 - Flags: feedback+
Attached patch patch v3Splinter Review
Sounds fine to me.
Attachment #8641375 - Attachment is obsolete: true
Attachment #8641489 - Flags: review?(karlt)
Attachment #8641489 - Flags: review?(karlt) → review+
Mike, I'm having trouble with Valgrind reporting leaks with this patch. Here is the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21d18fe98e6d

It *looks* like the leaks being reported are the ones that were suppressed in bug 1187649. Do you have any idea what is going on here?
Flags: needinfo?(mh+mozilla)
You appear to have based your try push on a broken state after some incomplete backout. The base changeset of your try push has valgrind builds red as well: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=a41bc7b8a24b
Rebase.
Flags: needinfo?(mh+mozilla)
Ah, thanks. Usually m-c is pretty safe to base on.
https://hg.mozilla.org/mozilla-central/rev/d1cdd1897bf1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.