Closed Bug 660131 Opened 10 years ago Closed 10 years ago

crash xpcshell with --enable-gio option on mochitest-plain

Categories

(Toolkit :: General, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: hiro, Assigned: hiro)

Details

(Whiteboard: [fixed in cedar])

Attachments

(2 files, 2 obsolete files)

When run mochitest-plain, http server xpcshell crashes.

#0  0x0000000000000000 in ?? ()
#1  0x00007fffef0bafc3 in g_content_type_guess (filename=0x2248828 "file.js", data=0x0, data_size=0, result_uncertain=0x7fffffff958c)
    at /build/buildd/glib2.0-2.28.6/./gio/gcontenttype.c:908
#2  0x00007fffe9bd0c6f in nsGIOService::GetMimeTypeFromExtension (this=<value optimized out>, aExtension=..., aMimeType=...)
    at /home/zoe/hg/mozilla-central/toolkit/system/gnome/nsGIOService.cpp:322
Attached patch Fix (obsolete) — Splinter Review
xpcshell does not depend on GTK+ so we need to call g_type_init in nsGIOService::Init() explicitly.
Attachment #535534 - Flags: review?(karlt)
This patch fixes nsGIOService, thanks.

nsGSettingsService also needs g_type_init to be called before it calls
g_settings_new.

In fact every service provided by kGnomeModule needs g_type_init to be called,
although this is done implicitly for two services:
nsAlertsIconListener::InitAlertAsync() calls notify_init() calls g_type_init().
nsGnomeVFSService::Init() calls gnome_vfs_init() calls g_type_init().

Still nsGConfService, nsGSettingsService, and nsGIOService need the explicit
call.  (Only nsGConfService::Init() already calls g_type_init()).

What do you think about instead initializing the loadProc of kGnomeModule to a static function that calls g_type_init?
http://hg.mozilla.org/mozilla-central/annotate/a18b9861a868/toolkit/system/gnome/nsGnomeModule.cpp#l114
http://hg.mozilla.org/mozilla-central/annotate/ce0dfa63db1b/xpcom/components/Module.h#l132

g_type_init could then be removed from nsGConfService, and
nsGIOService::Init() can be removed and
NS_GENERIC_FACTORY_CONSTRUCTOR_INIT replaced with
NS_GENERIC_FACTORY_CONSTRUCTOR.
(In reply to comment #2)
> This patch fixes nsGIOService, thanks.
> 
> nsGSettingsService also needs g_type_init to be called before it calls
> g_settings_new.
> 
> In fact every service provided by kGnomeModule needs g_type_init to be
> called,
> although this is done implicitly for two services:
> nsAlertsIconListener::InitAlertAsync() calls notify_init() calls
> g_type_init().
> nsGnomeVFSService::Init() calls gnome_vfs_init() calls g_type_init().
> 
> Still nsGConfService, nsGSettingsService, and nsGIOService need the explicit
> call.  (Only nsGConfService::Init() already calls g_type_init()).
> 
> What do you think about instead initializing the loadProc of kGnomeModule to
> a static function that calls g_type_init?
> http://hg.mozilla.org/mozilla-central/annotate/a18b9861a868/toolkit/system/
> gnome/nsGnomeModule.cpp#l114
> http://hg.mozilla.org/mozilla-central/annotate/ce0dfa63db1b/xpcom/components/
> Module.h#l132
> 
> g_type_init could then be removed from nsGConfService, and
> nsGIOService::Init() can be removed and
> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT replaced with
> NS_GENERIC_FACTORY_CONSTRUCTOR.

Wow! Thank you for your a sharp and an awesome suggestion. I did not know the  LoadFuncPtr.  I will attach a new patch to use it later.
Attached patch Revised patchSplinter Review
Unfortunately nsGConfService::Init() could not be removed since the method does also initialize mClient but this patch gets better than the previous one.
Assignee: nobody → hiikezoe
Attachment #535534 - Attachment is obsolete: true
Attachment #535534 - Flags: review?(karlt)
Attachment #536497 - Flags: review?(karlt)
Comment on attachment 536497 [details] [diff] [review]
Revised patch

Yes, looks good.  Thanks!
Attachment #536497 - Flags: review?(karlt) → review+
Keywords: checkin-needed
Pushed http://hg.mozilla.org/projects/cedar/rev/d6a897278194
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla7
Pushed:
http://hg.mozilla.org/mozilla-central/rev/d6a897278194
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Version: unspecified → Trunk
Does not compile:

.../toolkit/system/gnome/nsGnomeModule.cpp: In function 'nsresult InitGType()':
.../toolkit/system/gnome/nsGnomeModule.cpp:113:15: error: 'g_type_init' was not declared in this scope
make[5]: *** [nsGnomeModule.o] Error 1
Attached patch follow up fix (obsolete) — Splinter Review
(In reply to comment #8)
> Does not compile:
> 
> .../toolkit/system/gnome/nsGnomeModule.cpp: In function 'nsresult
> InitGType()':
> .../toolkit/system/gnome/nsGnomeModule.cpp:113:15: error: 'g_type_init' was
> not declared in this scope
> make[5]: *** [nsGnomeModule.o] Error 1

I am sorry for my fault. Are you disabling gconf, right?
Attachment #536971 - Flags: review?(karlt)
Attachment #536971 - Flags: review?(karlt) → review+
Keywords: checkin-needed
Yes, --disable-gconf is in effect. Unfortunately it does still not compile with the follow-up fix. Including <gconf/gconf-client.h> instead of <glib.h> works.
(In reply to comment #10)
> Yes, --disable-gconf is in effect. Unfortunately it does still not compile
> with the follow-up fix. Including <gconf/gconf-client.h> instead of <glib.h>
> works.

Ah! My dumb head! We should include glib/gobject.h actually.
Keywords: checkin-needed
Attached patch correct fixSplinter Review
I am sorry for my bonehead!

The correct header was glib-object.h.
Attachment #536971 - Attachment is obsolete: true
Attachment #537035 - Flags: review?(karlt)
Now I confirmed that building succeeded with --disable-gconf option.

Thank you, Stefan.
Attachment #537035 - Flags: review?(karlt) → review+
Keywords: checkin-needed
Follow-up pushed to cedar.
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
You need to log in before you can comment on or make changes to this bug.