Closed
Bug 1189125
Opened 9 years ago
Closed 9 years ago
Allow xpcshell to use GTK
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(1 file, 2 obsolete files)
3.02 KB,
patch
|
karlt
:
review+
|
Details | Diff | 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 1•9 years ago
|
||
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 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
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.
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Comment 12•9 years ago
|
||
Huh, I don't know why I read EnsureInitialized. Let's put that on exhaustion. This is better reviewed by Karl anyways.
Comment 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
Sounds fine to me.
Attachment #8641375 -
Attachment is obsolete: true
Attachment #8641489 -
Flags: review?(karlt)
Updated•9 years ago
|
Attachment #8641489 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
Ah, thanks. Usually m-c is pretty safe to base on.
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d1cdd1897bf1
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•