190560 - xpcom is unable to always load .js component file

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Sean Su]

Seth and I found a problem with xpcom trying to load a .js component from the
mozilla's component dir (as opposed to the GRE's component dir).

We were trying to fix bug 190465, and noticed that nsDownloadProgressListener.js
would only load consistently when it was located in the GRE's component dir.

If it was in the mozilla's component dir.  It would only load once, then the
feature would no longer work if you quit and restarted the browser.  Even this
one time was not always reproduceable.

Both seth and I were experiencing this, so I doubt it's something with my system
in particular.

Seth tested out other features that had .js components in the mozilla's
component dir.  They seem to have loaded fine.  So I'm not sure exactly what's
going on here.

Comment 1

[Doug Turner (:dougt)]

nor am I.

how are you registering this .js file?  

Comment 2

[Daniel Veditz [:dveditz]]

Not the registry, just XPCOM

Comment 3

[Doug Turner (:dougt)]

note that dan's comment was about the "component" attribute of this bug.

Comment 4

[Doug Turner (:dougt)]

this works for me.  

ssu/seth, can you reproduce this problem for me on monday?  If this is a
problem, I wanna know right away.

Comment 5

[Sean Su]

here are the steps with a new build that *contains* the fix to bug 190465:
1) make sure mozilla and GRE are not installed
2) install mozilla (default settings is what I used)
3) notice that doing a "File|Save Page As" (and actually saving the file) works.
4) quit mozilla when it runs at the end of the install
5) delete [mozilla]\components\*.dat
6) *move* the following files from [gre 1.3b]\components to [mozilla]\components:
     nsProgressDialog.js
     nsHelperAppDlg.js
     nsDownloadProgressListener.js
7) start mozilla
8) notice that doing a "File|Save Page As" (and actually saving the file) does
*not* work.

Dougt, if the steps above still do not reproduce this bug, I can swing by your
cube and help you reproduce this.

Comment 6

[Doug Turner (:dougt)]

where is xpc3250 in the install?  I do not see this file in any of the
compreg.dat files ssu has sent me via email.

Comment 7

[Doug Turner (:dougt)]

ignore that last comment.  

Comment 8

[Doug Turner (:dougt)]

duh!  found the problem.  

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.1

Keep track of the component loaders.  If there is an additional component
loader added during the registration of gre components, we autoreg the
application components directory again (for the new non-native loaders only).  


I am pretty sure that this works.  ssu, can you give it a swing?

Comment 10

[Alec Flett]

Comment on attachment 112836 [details] [diff] [review]
patch v.1

generally this looks fine, but can we keep the DEBUG_dougt stuff? I think we
have enough registration spew already.
Also, I see lots of NS_COM/NS_EXPORT stuff - is that intentional? necessary?

sr=alecf with the DEBUG_dougt left, and an explanation for the NS_EXPORT stuff

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 112836 [details] [diff] [review]
patch v.1

okay to the printf's.

Regarding the NS_EXPORT stuff, the function that I am removing this from isn't
used outside of XPCOM.	There is no reason to export it, nor (i guess) no real
reason include it with this bug.  It was just in my tree at the time I took the
diff.  If it is all the same, I would like to check this part in too.

Comment 12

[Sean Su]

Comment on attachment 112836 [details] [diff] [review]
patch v.1

xpinstall now errors out from AutoRegisterComponentByLoaderIndex().

It was not going into the for() loop.  fromLoaderType is 0.  mNLoaderData is 1.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 112836 [details] [diff] [review]
patch v.1

grumble.... wrong patch.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.2

this is the real deal.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 112864 [details] [diff] [review]
patch v.2

sorry about that, the only different between the two patchs is alecf's
suggestion regarding the printf, and i made that AutoRegisterByLoaderIndex more
encapsulated by not exposing the loader id in the method and called this new
method AutoRegisterNonNativeComponents.  This makes more sense.

Comment 16

[timeless]

Comment on attachment 112864 [details] [diff] [review]
patch v.2

>+    // registers only the files in spec's location by loaders other than the
>+    // native loader.  This is an optimization method only.
>+    nsresult AutoRegisterNonNativeComponents(nsIFile* spec);
the comment is wrong: if spec is null you intentionally check some other
directory :)

Comment 17

[Alec Flett]

Comment on attachment 112864 [details] [diff] [review]
patch v.2

sr=alecf

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 112864 [details] [diff] [review]
patch v.2

Approval granted with the proviso that you need to get r= from ssu (or someone
else up on what's going on here with GRE/etc).

Comment 19

[Sean Su]

Comment on attachment 112864 [details] [diff] [review]
patch v.2

still not working for xpinstall being triggered via xpistub.dll.

I'll talk with you off line to help you reproduce this.

Comment 20

[Doug Turner (:dougt)]

ssu - why are you missing this patch on some error that you are seeing in a
totally different area?  

Comment 21

[Sean Su]

because xpinstall requires xpcom, and xpinstall is used by the native installer.

Comment 22

[Doug Turner (:dougt)]

never mind... ssu found the problem.... i shouldn't have default the return
result to NS_ERROR_UNEXCEPTED.  ssu say r=

Comment 23

[Sean Su]

r=ssu for changing the default return value.

Comment 24

[Sean Su]

Comment on attachment 112864 [details] [diff] [review]
patch v.2

contingent on changing the default return value to NS_OK per our talk.

Comment 25

[Doug Turner (:dougt)]

Thanks everyone.

Checking in components/nsCategoryManager.cpp;
/cvsroot/mozilla/xpcom/components/nsCategoryManager.cpp,v  <-- 
nsCategoryManager.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in components/nsComponentManager.cpp;
/cvsroot/mozilla/xpcom/components/nsComponentManager.cpp,v  <-- 
nsComponentManager.cpp
new revision: 1.219; previous revision: 1.218
done
Checking in components/nsComponentManager.h;
/cvsroot/mozilla/xpcom/components/nsComponentManager.h,v  <--  nsComponentManager.h
new revision: 1.78; previous revision: 1.77
done
Checking in components/nsRegistry.cpp;
/cvsroot/mozilla/xpcom/components/nsRegistry.cpp,v  <--  nsRegistry.cpp
new revision: 1.72; previous revision: 1.71
done

Comment 26

[Sean Su]

this fix seems to be working.  I verified by doing a manual fix of bug 191048. 
the download manager dialog shows up and works after I move
nsDownloadProgressListener.js from GRE to Mozilla.