237745 - (app-startup) split app-startup from core appshell functionality

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect, P1)

Description

[Benjamin Smedberg]

I'm moving appshell out of tier 9, and need to keep nsIWindowWatcher and
implementation in tier 9, for xpinstall and other random crap (ugh, but I don't
have time/inclination to fix the interface to not suck). 
embedding/components/windowwatcher is the logical destination.

cls/bz, does this sound OK to you?

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is danm's thing more than mine...

Comment 2

[Dan M]

WindowMediator is very much an appshell thing. appshell is a good place for it
because it lives and breathes XUL Window. embedding/components is a bad place
for it because, if for no other reason, that would require a compilation
dependency on appshell in embedding/components. WindowMediator belongs in appshell.

If I understand you correctly, I think what you actually want to do is consider
making all the files spread out all over the codebase outside of appshell which
use WindowMediator, use WindowWatcher instead.

And be sure to check in with me before you get the inclination to make one of
these interfaces not suck, whichever one you were talking about.

Comment 3

[Benjamin Smedberg]

ok, here's the big picture:

the startup sequence for firefox is changing dramatically: overview is at
http://bdsmedberg.no-ip.org/single-profile/2004-02-25.html

This means that we need to fork nsIAppShellService.idl in the new toolkit,
because the current interface makes no sense for the new toolkit startup
sequence. So xpfe/appshell and toolkit/appshell are going to be forked and live
in tier 50 (the toolkit). However, there are some interfaces which currently
live in appshell that need to be in tier 9, because gecko internals depend on them:

nsIWindowMediator is used xpinstall for the xpinstall UI (a dependency that
could probably broken) and DOM for window.find (a dependency that might be
breakable, but I don't know enough to say).

This involves some dicing-up of appshell. We can put the nsIWindowMediator and
nsIXULWindow interfaces in embedding/components but leave the implementations
(forked) in xxx/appshell...

or we can break the xulwindow dependency on nsiwebshellwindow and move the
windowmediator and xulwindow impls into embedding/components.

(I also need to move nsIPopupWindowManager.idl, which is pretty simple because
appshell doesn't implement or use it anyway, bug 237744).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Whoa.  We're forking appshell?  Why?  Is it not possible to simply modify
appshell to do what's needed?  Why not?

Comment 5

[jag (Peter Annema)]

Perhaps you could attach your overview to this bug? I can't see it because it
"has been temporarily blocked", and as an attachment it'll probably enjoy a
longer life (for all future readers to see) than on an external server.

Comment 6

[Dan M]

I've read your proposal. Gotta say, replacing the "implementations we don't want
to try" paragraph with one that explains why it's a good idea in the first place
would have aided my comprehension. I gather this is something you've been
working on with Ben for a long time, so me quickly catching up is unlikely.

You may have heard, there's been a flurry of email about this project. I've said
my piece; I don't much like any of it. Despite my objections, assuming you go
ahead with separating the Window part of appshell from the rest, I suggest just
making that a new module in whichever build tier you need. I continue to believe
that the real fix is to make core code not dependent on XUL appshell code.
That's a bad dependency, anyway.

embedding/components is a terrible place for XULWindow et.al. Terrible. It goes
there over my dead body. And please don't split the interface files away from
the implementation. That's an unhappy build hack. And the XUL interface doesn't
belong in embedding any more than the implementation.

PS The sincerely chrome registry specific methods that are commented
"xxxbsmedberg Move me to nsIWindowMediator" only add to my discontent.

Comment 7

[Benjamin Smedberg]

Attachment: semi-single-profile proposal, for posterity

AOL network blocks your DNS lookups to my host :(

danm: Ben and I talked, and I think we all agree that the goal is to fork as
little as possible. Here's the alternative that currently looks best:

We keep xpfe/appshell in tier 9 (I would like to move the directory out of
xpfe/ at some point, but that's not really important). We split+fork the
following stuff into xpfe/components/startup or someplace equivalent:

nsICmdLine*
nsIAppSupport*
and the parts of nsIAppShellService that deal with startup sequencing (these
would move into a new interface "nsIXULAppStartup" or something like that):
  initialize
  doProfileStartup
  nativeAppSupport
  hideSplashScreen
  createStartupState
  ensure1Window

Comment 8

[Benjamin Smedberg]

Attachment: split appstartup out of appshell NOTE: IGNORE toolkit changes for the moment

This splits appstartup out of appshell. It builds and works in seamonkey. It
does *not* work in toolkit yet, but it will be simple to get it working in
toolkit once it's reviewed for seamonkey.

Comment 9

[Benjamin Smedberg]

Comment on attachment 144676 [details]
split appstartup out of appshell NOTE: IGNORE toolkit changes for the moment

please review ONLY the seamonkey portions of this patch... I will attach the
toolkit/profile-startup portions separately.

Comment 10

[Benjamin Smedberg]

Comment on attachment 144676 [details]
split appstartup out of appshell NOTE: IGNORE toolkit changes for the moment

This is a crappy patch... I've figured out a way to avoid all that toolkit
crud.

Comment 11

[Benjamin Smedberg]

Attachment: updated with, one JS error caught by beng, and without the toolkit cruft

Comment 12

[Benjamin Smedberg]

Attachment: document describing the implementation of this patch

Comment 13

[Benjamin Smedberg]

Comment on attachment 144688 [details]
updated with, one JS error caught by beng, and without the toolkit cruft

This is more reviewable ;)

I have only built and tested this on linux... I will build win32 tomorrow, and
upload to planetoid to test there.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

   [noscript]
-  void createHiddenWindow();
+  void createHiddenWindow(in nsIAppShell aAppShell);
+
+  void destroyHiddenWindow();


hmm, why is destroyHiddenWindow scriptable when createHiddenWindow isn't?

+    [noscript] void initialize(in long argc, out string argv);
whoa. is there a reason why [array, size_is(argc)] was avoided for argv? Yes,
you probably just copied it, but could you fix it?

Comment 15

[Benjamin Smedberg]

> hmm, why is destroyHiddenWindow scriptable when createHiddenWindow isn't?

I don't know why createHiddenWindow is [noscript]... probably historical
accident from when nsIAppShell was not a real interface. It doesn't matter...
even if you were doing something unusual like bootstrapping a browser from
xpcshell, you would have to go through the appstartup component, instead of
manually using nsIAppShellService.

> +    [noscript] void initialize(in long argc, out string argv);
> whoa. is there a reason why [array, size_is(argc)] was avoided for argv? Yes,
> you probably just copied it, but could you fix it?

size_is has to be an unsigned long. It's a stupid signature, as is
nsIAppShell.Init, but let's fix that later, shall we? I have a whole boatload of
potential cleanup I avoided for this patch. There's a decent amount of
deCOMification that can happen in appshell next cycle, as well.

Comment 16

[Benjamin Smedberg]

update: I had to make a few more changes for win32 and mac; I moved building the
activex control from tier 9 to tier 99 (it is now built with the rest of the
embedding "clients" like mfcembed). There were errors in
xpfe/components/build/Makefile.in where I was overriding makefile vars using =
instead of +=. And there were a couple places in mac-specific code where I
needed to do an nsIAppShellService->nsIAppStartup conversion
(nsNativeAppSupportMac and the appleevents code). And I had to change appstartup
to use a threadsafe isupports implementation, because we proxy it in a few places.

I can post a new patch, but none of those changes were major or dangerous...
also, if we can reach agreement on which files are going to move, I can go ahead
and get the files copied in CVS, then redo the patch with only the changed
portions of those files, which ought to make detailed reviewing a lot easier.

Comment 17

[Benjamin Smedberg]

Attachment: Readable, but not apply-able

This patch is readable, but not apply-able... I put the files which need to be
moved back in their original locations and took diffs from there.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Index: xpfe/bootstrap/nsNativeAppSupportWin.cpp
+    rv = compMgr->CreateInstanceByContractID(NS_COMMANDLINESERVICE_CONTRACTID,
+                                             nsnull,
+                                             NS_GET_IID(nsICmdLineService),
+                                             (void**)aResult );

why not use CallCreateInstance while you're here?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like you should have changes corresponding to the client.mk activex
changes to embedding/browser/activex/src/Makefile.in , but I don't see any in
the patch.  (Also, in the client.mk activex changes, the activex comment is
dangling a few lines after the activex stuff.)

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 145518 [details] [diff] [review]
Readable, but not apply-able

What biesi and dbaron said, plus my terse list o' issues:

Factory, etc. CamelCaps in nsComposerCmdLineHandler.js
#ifndef MOZ__THUNDERBIRD in msgMapiHook.cpp
nsIAppStartup.idl createHiddenWindow comment cut off
nsAppStartup::OpenBrowserWindow's body is overindented one c-basic-offset

Get mscott to look at the  MOZ_THUNDERBIRD changes and test 'em if you haven't.

Otherwise, the main thing is to test all the apps before landing.

/be

Comment 21

[Benjamin Smedberg]

Attachment: app-startup, revision 4

Nits picked, except the one about MOZ__THUNDERBIRD which I didn't understand.
This builds with tbird, and mscott said that I could land little mail/mailnews
changes like this without further review.

Comment 22

[Benjamin Smedberg]

Clarification: I added that tbird #ifdef because tbird doesn't have quicklaunch,
and will always have a profile when processing the eventloop. It therefore
doesn't need any special logic to "ensure" that there is a profile selected.

This patch needs to land together with the semi-single-profile work, because
this patch only covers the xpfe tine of the fork.

Comment 23

[Benjamin Smedberg]

Also, I had to revert back to compMgr->CreateInstanceByContractID instead of
using CallCreateInstance... that function links directly to the deprecated
nsComponentManager:: symbols, which won't work when we're using the XPCOM glue
in seamonkey.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 148129 [details] [diff] [review]
app-startup, revision 4

New, reduced trunk-only patch coming up?

/be

Comment 25

[Benjamin Smedberg]

Attachment: app-startup, revision 5

It's revived! This should build, with the same symlinks listed earlier in this
bug. I have not tested the changes to nsNativeAppSupportWin.cpp yes, that code
is untested but will be tested soon. Also I didn't build tbird, though the
changes there are minor. Will build before checkin ;)

Who wants to review? darin should, danm or dveditz, are you up to looking this
over?

Comment 26

[Darin Fisher]

Comment on attachment 160387 [details] [diff] [review]
app-startup, revision 5

>Index: editor/ui/nsComposerCmdLineHandler.js

>+  _factory: {
>+    createInstance: function(outer, iid) {
>+      if (outer != null) {
>+        throw R.NS_ERROR_NO_AGGREGATION;
>+      }
>+
>+      return new nsComposerCmdLineHandler();

	return new nsComposerCmdLineHandler().QueryInterface(iid);


more to come...

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think you probably mean

  return (new nsComposerCmdLineHandler()).QueryInterface(iid);

or something similar.

Comment 28

[Brendan Eich [:brendan]]

shaver, darin had it right:

  return new nsComposerCmdLineHandler().QueryInterface(iid);

ECMA-262 goes out of its way to make this work.  See the productions

  NewExpression :
    MemberExpression
    new NewExpression

and

  MemberExpression :
    new MemberExpression Arguments

and

  CallExpression :
    MemberExpression Arguments

Once a new foo(args) has been reduced to a MemberExpression, subsequent . or []
operators use that MemberExpression as left operand.  No need to parenthesize
the new expression with arguments.

/be

/be

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Well, blow me over.  Apologies!

Comment 30

[Darin Fisher]

moreover, it works just fine in practice.  i use that pattern all the time ;)

Comment 31

[Darin Fisher]

Attachment: review comments for "app-startup, revision 5"

just a bunch of nits

Comment 32

[Benjamin Smedberg]

> why is this interface being forked?  why don't we make seamonkey
> inherit the interface from toolkit?  or do you mean to remove the

Actually, forking the interface was the whole point of this exercize. The many
methods for dealing with the splash screen, profile migration, and
command-line-handling need to change (rapidly and radically, as we've
discussed), and rewriting the seamonkey profile migrator is likely to be the
very last step in porting it to toolkit. We should consider
xpfe/components/startup a short-term solution until seamonkey gets its porting
in gear.

Comment 33

[Darin Fisher]

> We should consider xpfe/components/startup a short-term solution until seamonkey 
> gets its porting in gear.

ok, works for me.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

>Also, I had to revert back to compMgr->CreateInstanceByContractID instead of
>using CallCreateInstance... that function links directly to the deprecated
>nsComponentManager:: symbols, which won't work when we're using the XPCOM glue
>in seamonkey.

you could make CallCI use NS_GetComponentManager! ;)

(quoting review comment)
>>+      url.AssignWithConversion(urlToLoad);
> nit: use CopyASCIItoUTF16 instead

Absolutely! please do that :) (or AssignASCII, given your IsASCII check)

> splash screen

will toolkit offer some way to show a splash screen? aiui it doesn't have a way now.

(quoting patch v5)
client.mk
+# Configure
+
+configure:: $(OBJDIR)/Makefile

why this dependency? configure creates the makefile surely, not depends on it?
ah... so "make -f client.mk" would trigger a configure run if needed?

nsComposeCmdLineHandler.js:
+const R = Components.results;

hm, I don't think this is common... I'm not sure it increases readability

+                appStartup->Quit(nsIAppStartup::eAttemptQuit);

app _startup_ quits the app? heh.

nsSystemPref.cpp
+        if (!gSysPrefLog) return NS_ERROR_OUT_OF_MEMORY;

please put the return on a second line

profile/build/Makefile.in
-GRE_MODULE	= 1
(also public/)

?

profileSelection.js:
+        var appStartup =
Components.classes["@mozilla.org/seamonkey/app-startup;1"].
+                           getService(Components.interfaces.nsIAppStartup);

usual style is:
+        var appStartup = Components.classes["@mozilla.org/seamonkey/app-startup;1"]
+                               .getService(Components.interfaces.nsIAppStartup);

with the dot from getService under the dot from .classes, which I'm sure I did
wrong in this comment


Yeah... why kill nsAppShellCIDs.h? put the contracts there, rather that into the
idl :)

nsIAppShellService.idl:
   nsIXULWindow createTopLevelWindow(in nsIXULWindow aParent,

this function has documentation (hard to see in the patch), mind updating it to
explain your new nsIAppShell arg?

nsXULWindow
+     obssvc->NotifyObservers(nsnull, "xul-window-visible", nsnull);

nice, this could probably be used by gnome's libappstartup-notification (or
whatever that's called)

xpfe/bootstrap/nsAppRunner.cpp
+    do_GetService(NS_COMMANDLINESERVICE_CONTRACTID,&rv);

missing space before &rv


xpfe/browser/src/nsBrowserInstance.cpp
+      rv = pIProxyObjectManager->GetProxyForObject(NS_UI_THREAD_EVENTQ,
NS_GET_IID(nsIAppStartup),
+                                                appStartup, PROXY_ASYNC |
PROXY_ALWAYS,
+                                                getter_AddRefs(appStartupProxy));

while you are touching this line, you could fix its indentation...

xpfe/components/build/Makefile.in
 	$(DIST)/lib/$(LIB_PREFIX)related_s.$(LIB_SUFFIX) \
+	../startup/src/$(LIB_PREFIX)appstartup_s.$(LIB_SUFFIX) \

hmm, why is this not using $DIST/lib?

> Reviewers made me add this comment.

hehe.

xpfe nsAppStartup.cpp:
+        NS_ASSERTION(0, "Failed to get a platform charset");

make that NS_ERROR, please

+    printf("default args: %s\n", NS_ConvertUCS2toUTF8(defaultArgs).get());

maybe UCS2 -> UTF16

+    nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow));
+    if (thing)
+      thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval);

CallGetInterface?

(sorry if all this is copied. it shows with + in the patch! ;) )

+      var cmdLineService = Components.classes[
"@mozilla.org/app-startup/commandLineService;1" ]

the appstartup service has /toolkit/ in its contractid, but cmdline is in
/app-startup/? why not put both into /app-startup/, the service as
/app-startup/service;1 or something?

toolkit/xre/nsINativeAppSupport.idl
+ * The interface provides these functions:

now that's an interesting way to document the methods. anyway, not your fault.
but maybe you want to fix it anyway? ;) (i.e. put the comments for each function
directly in front of it)

Comment 35

[Benjamin Smedberg]

> you could make CallCI use NS_GetComponentManager! ;)

Followup bug 263360 filed.

> will toolkit offer some way to show a splash screen? aiui it doesn't have a
way now.

No, unless there is great wailing and gnashing of teeth.

> (quoting patch v5)
> client.mk
> +# Configure
> +
> +configure:: $(OBJDIR)/Makefile
> 
> why this dependency? configure creates the makefile surely, not depends on it?
> ah... so "make -f client.mk" would trigger a configure run if needed?

Actually, configure is a "fake" target, so that I can get client.mk to run
configure without doing any more building steps. e.g. "make -f client.mk configure"

> profile/build/Makefile.in
> -GRE_MODULE	= 1
> (also public/)
> 
> ?

Yes. nsIProfile.idl, though frozen, should not be part of the GRE, since it is
not part of the toolkit, only part of seamonkey.

> xpfe/components/build/Makefile.in
>  	$(DIST)/lib/$(LIB_PREFIX)related_s.$(LIB_SUFFIX) \
> +	../startup/src/$(LIB_PREFIX)appstartup_s.$(LIB_SUFFIX) \
> 
> hmm, why is this not using $DIST/lib?

Because I am not exporting this lib. Basically, I want to stop exporting a lot
of the intermediate libs that we use to build larger libs. It helps save a lot
of disk space on win32, and it's gratuitous.

> +    nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow));
> +    if (thing)
> +      thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval);
> 
> CallGetInterface?

This was copied, and I can't figure out how to make CallGetInterface work.

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #35)
> > ah... so "make -f client.mk" would trigger a configure run if needed?
> 
> Actually, configure is a "fake" target, so that I can get client.mk to run
> configure without doing any more building steps. e.g. "make -f client.mk
configure"

whoops, that's what I meant to write. but it will not always run configure,
right? maybe that'd be more useful?

> Because I am not exporting this lib. Basically, I want to stop exporting a lot
> of the intermediate libs that we use to build larger libs. It helps save a lot
> of disk space on win32, and it's gratuitous.

ah, makes sense.

> > +    nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow));
> > +    if (thing)
> > +      thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval);
> > 
> > CallGetInterface?
> 
> This was copied, and I can't figure out how to make CallGetInterface work.

(rv = ) CallGetInterface(thing.get(), _retval);

(I'm not quite sure whether the .get() is needed...)

oh, nevermind. that won't help with a void**.

Comment 37

[[e-mail]]

Comment on attachment 160387 [details] [diff] [review]
app-startup, revision 5

>+const C = Components.classes;
>+const I = Components.interfaces;
>+const R = Components.results;
These aren't prevalent style. Declaring a particular interface or result, e.g.
const nsISupports = Components.interfaces.nsISupports is preferred.

>+  _CID: Components.ID("{f7d8db95-ab5d-4393-a796-9112fe758cfa}"),
>+  _contractIDPrefix: "@mozilla.org/commandlinehandler/general-startup;1?type=",
These should be top-level consts. The scope is private to the component anyway.

>+  _factory: {
>+    createInstance: function(outer, iid) {
>+      if (outer != null) {
>+        throw R.NS_ERROR_NO_AGGREGATION;
>+      }
>+
>+      return new nsComposerCmdLineHandler();
>+    }
This could be a top-level object too.

>+    if (!iid.equals(I.nsICmdLineHandler) &&
>+        !iid.equals(I.nsISupports)) {
>+          throw R.NS_ERROR_NO_INTERFACE;
>+    }
You braced this but not the one above... I'm no fan of braces but this is a new
file so you get to choose the style, as long as you're consistent.

>+  get defaultsArgs() {
Nit: should be defaultArgs. While I mention it, you could probably use JS
properties rather than writing each getter out, it's not as if anyone's going
to be able to change these values.

>+    catMan.deleteCategoryEntry("command-line-argument-handlers",
>+                               "nsComposerCmdLineHandler", true);
Looks like this will be the first command line handler to unregister itself
correctly!

>+    rv = compMgr->CreateInstanceByContractID(
>+                    NS_COMMANDLINESERVICE_CONTRACTID,
>+                    nsnull, NS_GET_IID(nsICmdLineService),
>+                    (void**) aResult);
I don't claim to have followed the create instance debate but I assume at some
point this will become rv =
CallCreateInstance(NS_COMMANDLINESERVICE_CONTRACTID, aResult);

>+    nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow));
>+    if (thing)
>+      thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval);
Bah, why is it you can do nsCOMPtr<nsIWebBrowserChrome> wbc =
do_GetInterface(newWindow) but not CallGetInterface(newWindow, _retval); :-/

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

> oh, nevermind. that won't help with a void**.

actually do mind, as _retval is NOT a void** - so CallGetInterface should work

Comment 39

[Benjamin Smedberg]

Fixed on trunk. Onwards to command-line handling!

Comment 40

[[e-mail]]

>+    if (!iid.equals(I.nsICmdLineHandler) &&
>+        !iid.equals(I.nsISupports)) {
>+          throw R.NS_ERROR_NO_INTERFACE;
>+    }
Bah, I failed to notice the bad indenting of "throw" :-[

>+  get defaultsArgs() {
>+    return "about:blank"
>+  },
I was hoping for defaultArgs: "about:blank", :-/

Comment 41

[[e-mail]]

Doh, I also overlooked this :-[
>+  var appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"].
>+                     getService(Components.interfaces.nsIAppStartup);
which should of course line up .getService with .classes

Comment 42

[Peter Weilbacher]

Attachment: Final build bustage fix for OS/2

On top of the build bustage fixes that dbaron already checked in this gets it
working again for OS/2. Can someone check this in?

Comment 43

[Scott MacGregor]

The Thunderbird windows build is still busted on Tinderbox from these changes.
Can you take a look? Re-opening until the bustage clears...Thanks!

Comment 44

[Scott MacGregor]

Not trying to be a pest, but the Thunderbird windows trunk build is still in
flames from this checkin. It's been a couple days:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird

any chance of an ETA on when we can expect to get Windows builds again?

Thanks!
-Scott

Comment 45

[Simon Paquet [:sipaq]]

This change also busted the Sunbird build process.
I get the same error message as the WINNT 5.0 patrocles Clbr Tbird tinderbox.

Although it is not directly related to this bustage, the change to splash.rc
(and other changes as appropriate) should probably also be made for Sunbird.

Comment 46

[David G King]

As this is causing "red" on patrocles and prevents cvs builds from compiling
Thunderbird and Sunbird changing to "blocker".

Comment 47

[Stephen Walker]

Attachment: MOZ_XUL_APP winhooks bustage fix

Comment 48

[David G King]

(In reply to comment #47)
> Created an attachment (id=164125)
> MOZ_XUL_APP winhooks bustage fix

With this fix in my Thunderbird tree, I now get :-

Creating Resource file: module.res
/cygdrive/e/mozilla/source/thunderbird/mozilla/build/cygwin-wrapper windres -O c
off --use-temp-file -DMOZ_THUNDERBIRD --include-dir /cygdrive/e/mozilla/source/t
hunderbird/mozilla/mail/app -DTHUNDERBIRD_ICO=\"../../dist/branding/thunderbird.
ico\" -DOSTYPE=\"WINNT5.1\" -DOSARCH=\"WINNT\" -DAPP_VERSION=\"0.6+\" -DBUILD_ID
=\"0000000000\" --include-dir ../../dist/include/string --include-dir ../../dist
/include/xpcom --include-dir ../../dist/include/xulapp --include-dir ../../dist/
include/xpinstall --include-dir ../../dist/include/appshell --include-dir ../../
dist/include --include-dir ../../dist/include --include-dir ../../dist/include/n
spr -o module.res /cygdrive/e/mozilla/source/thunderbird/mozilla/mail/app/module
.rc
e:/mozilla/source/thunderbird/mozilla/mail/app/module.rc:84:35: nsNativeAppSuppo
rtWin.h: No such file or directory
e:\gnu\mingw\bin\windres.exe: e:\gnu\mingw\bin\gcc exited with status 1
make[4]: *** [module.res] Error 1
make[4]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla/mail/
app'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla/mail'

make[2]: *** [libs] Error 2
make[2]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla'
make: *** [alldep] Error 2

Comment 49

[Stephen Donner [:stephend] Not actively reading bugmail]

Just having a trunk Seamonkey Mail window with no Navigator window, trying to
_get_ a Navigator window fails, and yields:

Error: uncaught exception: [Exception... "Could not convert JavaScript argument
(NULL value cannot be used for a C++ reference type) arg 0
[nsISupports.QueryInterface]"  nsresult: "0x8057000b
(NS_ERROR_XPC_BAD_CONVERT_JS_NULL_REF)"  location: "JS frame ::
chrome://communicator/content/tasksOverlay.js :: OpenBrowserWindow :: line 120"
 data: no]

Warning: reference to undefined property Components.interfaces.nsICmdLineHandler
Source File: chrome://communicator/content/tasksOverlay.js
Line: 120

Trying to quit the app via File | Quit fails as well, and yields:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]"  nsresult: "0x80570018
(NS_ERROR_XPC_BAD_IID)"  location: "JS frame ::
chrome://global/content/globalOverlay.js :: goQuitApplication :: line 23"  data: no]

Warning: reference to undefined property Components.interfaces.nsIAppStartup
Source File: chrome://global/content/globalOverlay.js
Line: 23

Comment 50

[Stephen Walker]

David, does adding LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/xre to
mail/app/Makefile.in fix the bustage?  My VC8 build is busted in addrbook
currently, so I couldn't verify if anything further along built before attaching
the previous patch.

Comment 51

[David G King]

That fix allows my build to finish OK.

(In reply to comment #50)
> David, does adding LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/xre to
> mail/app/Makefile.in fix the bustage?  My VC8 build is busted in addrbook
> currently, so I couldn't verify if anything further along built before attaching
> the previous patch.

Comment 52

[Benjamin Smedberg]

stephend, are you using an installer build? I think I probably forgot to add
some .xpt files to the packaging manifests which would cause the error you're
describing.

Comment 53

[Benjamin Smedberg]

I fixed mail/app/Makefile.in adding the LOCAL_INCLUDES line, and fixed the
installer packaging for seamonkey/firefox/thunderbird. I'm going to mark this
bug FIXED. If there are additional issues, can you open a separate bug (mark the
dependency) so that I can keep track of things?

Comment 54

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #52)
> stephend, are you using an installer build? I think I probably forgot to add
> some .xpt files to the packaging manifests which would cause the error you're
> describing.

Yes, I am using an installer build, and upgrading to 2004-11-01-04 fixed this.

Thanks.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix reliable crash-on-shutdown caused by previous patches

I've been crashing on shutdown for a while, in js_PurgeDeflatedStringCache. 
This fixes it.

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Which was essentially a regression of bug 249737, so I'm just going to check it
in.  Never mind the reviews.  That's what you get for copying an old copy of a
file and just checking it in.)

Comment 57

[Brendan Eich [:brendan]]

Darin saw the js_PurgeDeflatedStringCache signature too -- thanks for finding
the culprit, dbaron.

/be

Comment 58

[Andy Willis (abwillis)]

I received:
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:883: warning: invalid

   conversion from `const char* const' to `char*'
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp: At global scope:
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:943: prototype for `
   nsresult nsNativeAppSupportOS2::Quit()' does not match any in class `
   nsNativeAppSupportOS2'
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:303: candidate is:
   virtual void nsNativeAppSupportOS2::Quit()
E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:943: `nsresult
   nsNativeAppSupportOS2::Quit()' and `virtual void
   nsNativeAppSupportOS2::Quit()' cannot be overloaded
make.exe[4]: *** [nsNativeAppSupportOS2.o] Error 1
make.exe[4]: Leaving directory `E:/cvs/work/mozilla/sbobj/toolkit/xre'
make.exe[3]: *** [libs] Error 2
make.exe[3]: Leaving directory `E:/cvs/work/mozilla/sbobj/toolkit'
make.exe[2]: *** [tier_50] Error 2
make.exe[2]: Leaving directory `E:/cvs/work/mozilla/sbobj'
make.exe[1]: *** [default] Error 2
make.exe[1]: Leaving directory `E:/cvs/work/mozilla/sbobj'
make.exe: *** [build] Error 2

building FireFox and Sunbird (clean builds).  Is this because the Final build
bustage for OS/2 patch has not been commited?
Andy
P.S.  I would test it but as I am building the suite right now (which doesn't
have this issue) it will be tomorrow before I can start a build with the patch
in place and hours from then before I would be certain if this was a fix for
this issue.  Therefore I am not reopening unless I test it before I hear back here.

Comment 59

[Peter Weilbacher]

Comment on attachment 163959 [details] [diff] [review]
Final build bustage fix for OS/2

This never went in like that.

Comment 60

[Andy Willis (abwillis)]

This was not checked into the the Aviary Branch for OS/2 (neither was 258217). 
I found issue was NS_IMETHOD Quit; was changed to void Quit();
Changing it back cleans up this error when building Firefox and Sunbird on OS/2
from the Trunk.  As I get an error from 258217 I can't say for certain that it
fixes everything but as best I can tell it should.  Not sure why it was switched
to void as the windows version was not changed so I don't know if it was a
mistake or intentional but something else wasn't done to complete that change.

Comment 61

[Ian Neal]

Relanded relevant parts of patch for globalOverlay.js and
nsExtensionManager.js.in following landing of aviary branch.