586523 - New libxpcomglue_s isn't compatible with old versions of Firefox due to mozalloc dependency

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew Zitnay]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)

The changes to xpcomglue_s in xulrunner-sdk 2.0 have caused it to require the new mozalloc.  This means any XPCOM component for use in an extension can currently only be compatible with either Firefox < 4.0, or Firefox >= 4.0, but not both.  This is far from ideal.  See bug #577831 and http://groups.google.com/group/mozilla.dev.extensions/browse_thread/thread/ebc2b3d1eb842660 for more details.

Reproducible: Always

Steps to Reproduce:
1. Create an XPCOM component using xulrunner-sdk 2.0 that links against xpcomglue_s (and necessarily, mozalloc)
2. Include it in an extension
3. Try to install the extension in Firefox < 4.0
Actual Results:  
XPCOM component doesn't load due to missing mozalloc

Expected Results:  
It should be possible for XPCOM components to be compatible with both old and new versions of Firefox

Benjamin Smedberg proposed a fix involving creating a new xpcomglue_s_nomozalloc that developers can use instead, to keep their XPCOM components compatible with both old and new versions of Firefox.  I will attach such a patch.

Comment 1

[Andrew Zitnay]

Attachment: Create xpcomglue_s_nomozalloc as well (same as xpcomglue_s but without a dependency on mozalloc)

Comment 2

[[e-mail]]

Attachment: git diff with renames for clarity

Comment 3

[[e-mail]]

Comment on attachment 465164 [details] [diff] [review]
git diff with renames for clarity

I'm not a build peer so these might be misunderstandings on my part.

>-DIRS            = standalone
>+DIRS            = standalone nomozalloc
I don't think this belongs in common.mk

> SDK_LIBRARY     =                        \
>-		$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
>-		$(NULL)
>+                $(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
>+                $(NULL)
This change looks wrong for nomozalloc/Makefile.in

>+# Don't use STL wrappers here (i.e. wrapped <new>); they require mozalloc
>+STL_FLAGS       =
If you moved this you could probably move more stuff into common.mk

Comment 4

[Andrew Zitnay]

Thanks Neil, I agree with all your comments.  I'm testing a new patch that incorporates them now, and will upload when done.

Comment 5

[Andrew Zitnay]

Attachment: Updated patch mostly for changes recommended by Neil

Updated patch to create xpcomglue_s_nomozalloc, brought to you by LastPass.com

Comment 6

[Benjamin Smedberg]

Comment on attachment 465269 [details] [diff] [review]
Updated patch mostly for changes recommended by Neil

There is already a "common.mk", it's called objs.mk. You don't need to move any of the EXPORTS/SDK_HEADERS stuff, since the headers only need to be exported once when we're building xpcom/glue.

Comment 7

[Andrew Zitnay]

Attachment: Changes to avoid creating common.mk

Comment 8

[damosuzuki]

I wasn't able to build with the above patch, as it fails with the following:

>LINK : fatal error LNK1181: cannot open input file 'pldhash.obj'

I think your Makefile.in is missing the following lines which actually copy the source files from glue/ to glue/nomozalloc/. Something like the following (adapted from glue/standalone):

>export:: $(XPCOM_GLUE_SRC_CSRCS) $(XPCOM_GLUE_SRC_CPPSRCS) >$(XPCOM_GLUENS_SRC_CPPSRCS) $(topsrcdir)/xpcom/glue/DeadlockDetector.h >$(topsrcdir)/xpcom/glue/SSE.h $(topsrcdir)/xpcom/glue/GenericModule.cpp >$(topsrcdir)/xpcom/glue/nsStringAPI.cpp
>	$(INSTALL) $^ .

Comment 9

[Andrew Zitnay]

Hmm, I worked around having to copy the source files by setting the srcdir to the parent dir...  I've been able to successfully build with this patch on WINNT_x86, Darwin_x86, Linux_x64_64, and Linux_x86.  Can anyone else reproduce this build problem?

Comment 10

[damosuzuki]

Ah, I see. Well, your patch with the additional "export::" command builds successfully for me. However, components built with xpcomglue_s_nomozalloc crash FF 3.6 and 4.0 when the component is registered. (Crash seems to occur in  mozilla::GenericModule::RegisterSelf.)

Have you been able to create working components with this version of glue?

Comment 11

[Andrew Zitnay]

Yeah, I've successfully tested our extension's XPCOM component linked against xpcomglue_s_nomozalloc with Firefox 3.6 and 4.0 under both WINNT_x86 and Darwin_x86 (haven't tried Linux yet).  It definitely registered and didn't crash, but I haven't tested it too far beyond that yet.

Comment 12

[[e-mail]]

Comment on attachment 465632 [details] [diff] [review]
Changes to avoid creating common.mk

This should probably have still been done as a git-style diff, so that you see the differences between nomozalloc/Makefile.in and the original Makefile.in

Comment 13

[Andrew Zitnay]

As a follow-up, I tested both Linux_x86 and Linux_x86_64, and was able to register an XPCOM component in both Firefox 3.6 and 4.0 without any issues or crashes.

Comment 14

[Benjamin Smedberg]

Comment on attachment 465632 [details] [diff] [review]
Changes to avoid creating common.mk

I don't think that changing the normal meaning of $srcdir is a good idea. Instead, please use the normal srcdir = @srcdir@ and add VPATH += $(srcdir)/..

Looks really close!

Comment 15

[Andrew Zitnay]

Attachment: Use export:: instead of changing srcdir and VPATH

Ok, I've made changes to add an export:: line instead of changing srcdir and VPATH.

Comment 16

[Benjamin Smedberg]

Comment on attachment 466313 [details] [diff] [review]
Use export:: instead of changing srcdir and VPATH

a=me to land this after beta4

Comment 17

[Bryan McQuade]

Thanks Andrew for the patch. I just want to verify that the glue_s_nomozalloc will be included with xulrunner sdk builds so I can use the sdk and not have to do my own sdk builds.

Comment 18

[Andrew Zitnay]

It's certainly the goal of myself and the rest of the folks here at LastPass that this patch make it into the Mozilla source tree, so that ourselves as well as other developers have an easy way to make XPCOM components that are compatible with past, present, and future versions of Firefox.

Benjamin is the XPCOM module owner, so it's his call.  It appears from his previous comment that he's planning on adding it after Firefox 4.0 beta 4 is released.  I don't know offhand when that will be.

Comment 19

[Bryan McQuade]

https://wiki.mozilla.org/Firefox/4/Beta shows that beta4 is targeting Aug 20, and that most milestones have missed by a few days. So maybe within a week we'll see this land.

Benjamin does that sound about right to you (1 week to land this)? Also, can the nomozalloc standalone glue get built by default so it's bundled with the released SDKs?

Comment 20

[Andrew Zitnay]

Looks like beta 4 was released today...  Hopefully we can get this patch into the tree soon :).

Comment 21

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/d8b1f82a7d89

Comment 22

[Benjamin Smedberg]

Thanks for taking this, Andrew.

Comment 23

[Benjamin Smedberg]

Andrew/Bryan, do one of you want to take a stab at creating docs for this on MDC? I think they should probably live on their own page, since this is a special hack for Firefox 4 that won't be maintained long-term.

Comment 24

[Andrew Zitnay]

Thanks for getting the patch into the tree, Benjamin.  I've taken a first stab at the docs:

https://developer.mozilla.org/en/XPCOM_Glue/XPCOM_Glue_without_mozalloc

and linked to the new page from near the bottom of:

https://developer.mozilla.org/en/XPCOM_Glue

where it talks about mozalloc.

Comment 25

[Benjamin Smedberg]

Good enough, thanks!