576606 - [OS/2] Fix registration follow-up to bug568691

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a6pre) Gecko/20100628 Minefield/3.7a6pre
Build Identifier: 

Registration in OS/2 widget code has to be updated after check-ins for bug568691

Reproducible: Always

Comment 1

[Walter Meinl]

Attachment: update nsWidgetfactory and nsRwsService

similar to what nsWidgetFactory was changed in gtk2 and mac directories

Comment 2

[Walter Meinl]

(In reply to comment #1)
> Created an attachment (id=455750) [details]
It appears that this patch is not enough. I have an additional patch for nsLocalFileOS2{h,cpp} and will investigate if even more changes have to be made. I'm not sure if "NS_IMETHODIMP" in nsRwsService.cpp have to be changed to "nsresult".
Nevertheless, the main problem for which I have not yet found a solution is during linkage of brwsrcmp.dll. Then I get an unresolved symbol _NSGetModule. We export _NSGetModule if a dll is built with _declspec(dllexport) (in rules.mk). Not yet sure if we can omit to export _NSGetModule or if we have to export sth else.

Comment 3

[Rich Walsh]

(In reply to comment #2)


> It appears that this patch is not enough. I have an additional patch for
> nsLocalFileOS2{h,cpp} and will investigate if even more changes have to be
> made. I'm not sure if "NS_IMETHODIMP" in nsRwsService.cpp have to be
> changed to "nsresult".

I think this just resolves to 'nsresult' on OS/2.  On Win, I think it becomes 'nsresult stdcall'. (I'll check).

> Nevertheless, the main problem for which I have not yet found a solution
> is during linkage of brwsrcmp.dll. Then I get an unresolved symbol
> _NSGetModule.  We export _NSGetModule if a dll is built with
> _declspec(dllexport) (in rules.mk). Not yet sure if we can omit to export
> _NSGetModule or if we have to export sth else.

NSGetModule is now obsolete.  See the "Binary Components" section of https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_1.9.3

I'm about to do a pull & a full rebuild of FF.  I'll apply your patches and see where it dies.

Comment 4

[Rich Walsh]

(In reply to comment #3)

> I'm about to do a pull & a full rebuild of FF.  I'll apply your patches
> and see where it dies.

I got to about a minute from completion when it died on brwsrcmp.dll.  The problem is in rules.mk line 1204 where we echo _NSGetModule into the .def file as the dll's only export.  Obviously this has to go, but I don't know if we have to explicitly export the new mozilla::Module structure.  It's way too late for me to find out now - I'll try it tomorrow.

BTW... I didn't get any explicit errors for nsLocalFileOS2.

Comment 5

[Rich Walsh]

Removing lines 1203 & 1204 in rules.mk solved the problem for brwsrcmp.dll (which is FF's only component dll).  The new '_NSModule' symbol was exported automatically, so we shouldn't have to add anything.  I'm going to try a debug build with "--disable-libxul" to look for problems when all of the individual component dlls are created.

 ifndef MOZ_OS2_USE_DECLSPEC
	$(FILTER) $(OBJS) $(SHARED_LIBRARY_LIBS) >> $@
 endif	
-else
-	echo    _NSGetModule >> $@
 endif
 else
 ifndef MOZ_OS2_USE_DECLSPEC

Comment 6

[Walter Meinl]

(In reply to comment #5)
> -else
> -    echo    _NSGetModule >> $@
A bit of history. This was added long time ago to rules.mk in order to prevent that (most of) the components would include all exported symbols. In fact, it's an override for the def file. I've diffed the map files of brwsrcmp from the new build (omitting that line) and a 1.9.2 build and indeed now I find _NSModule where the 1.9.2 map file shows _NSGetModule. Since the new registration process appears to export only this single symbol, it's probably the best to remove this line. Moreover, when we remove that line we can also clean up the whole ifdef section related to components. I tested it and it worked for a non-debug build, I'll wait on your debug results.

Comment 7

[Rich Walsh]

No problems that are directly related with my debug build.  I did find that a component dll, 'extensions.dll', has a long name that prevents it from loading.  See bug 576792.

Comment 8

[Walter Meinl]

Attachment: update nsWidgetfactory nsRwsService nsLocalFileOS2 and rules.mk

same patch for nsWidgetfactory and RwsService. I added the changes to nsLocalFileOS2 for the sake of consistency with the other platforms (though I didn't see a warning from the compiler, too) and finally replacing _NSGetModule by _NSModule in rules.mk for component dlls. With the latter change we can leave the remainder of the ifdef section as it is.

Comment 9

[Rich Walsh]

(In reply to comment #8)

> replacing _NSGetModule by _NSModule in rules.mk for component dlls. With the
> latter change we can leave the remainder of the ifdef section as it is.

Why substitute something that's unnecessary for something that's obsolete?  In fact, why retain any of these |ifndef MOZ_OS2_USE_DECLSPEC| entries?  Are we likely to ever go back to the old way of specifying exports?  If you don't want to deal with that now, OK, but every component builds (and runs) just fine without specifying any exports.

Also, in nsWidgetFactory.cpp

>+  NULL,
>+  NULL,
>+  nsAppShellInit,
>+  nsAppShellShutdown
>+};
>+
>+NSMODULE_DEFN(nsWidgetOS2Module) = &kWidgetModule;

Where you have 'nsAppShellShutdown', it should be 'nsWidgetOS2ModuleDtor'.  That function frees various PM resources, then calls nsAppShellShutdown.

Comment 10

[Rich Walsh]

BTW... js/src/config/rules.mk is never used to build a component (AFAIK).  If so, then there's no point in it having any component-oriented rules.  Again, if you'd rather not deal with that now, OK, but I thought it was worth mentioning.

Comment 11

[Walter Meinl]

(In reply to comment #10)
> BTW... js/src/config/rules.mk is never used to build a component (AFAIK).  If
> so, then there's no point in it having any component-oriented rules. 
Well, there is a tree-rule that rules.mk and config.mk (and others) in the main tree and the js subdir should be kept in sync. Therefore, even unnecessary rules may be contained in any of these files.
A while ago I've made a patch in my repo that removed all the declspec checks and anything that won't work without declspec. Somehow, I got retracted. I'll file a new bug and get rid of the components rules there.

Comment 12

[Walter Meinl]

Attachment: v2 update nsWidgetfactory nsRwsService nsLocalFileOS2 and rules.mk

(In reply to comment #9)

> Also, in nsWidgetFactory.cpp
> 
> >+  NULL,
> >+  NULL,
> >+  nsAppShellInit,
> >+  nsAppShellShutdown
> >+};
> >+
> >+NSMODULE_DEFN(nsWidgetOS2Module) = &kWidgetModule;
> 
> Where you have 'nsAppShellShutdown', it should be 'nsWidgetOS2ModuleDtor'. 
> That function frees various PM resources, then calls nsAppShellShutdown.

addressed

Comment 13

[Rich Walsh]

Comment on attachment 456099 [details] [diff] [review]
v2 update nsWidgetfactory nsRwsService nsLocalFileOS2 and rules.mk

Given the changes you're making in bug 577011, this patch is fine for now.

Comment 14

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/5a04fc36fdd9