416886 - JavaScript components in extensions should be registered last

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kathleen :Brade]

Attachment: debug log (extra debugging in nsComponentManager.cpp)

I have a Firefox extension that includes a JavaScript component that registers its own JavaScript global property handler (overriding one provided by Firefox).  In Firefox 2, my component is always registered after all of the Firefox components and everything works fine.  In Firefox 3, my component is registered before the Firefox component it is trying to replace.

I believe that JavaScript components in extensions should be registered last.  I added some debug logging to a trunk build; I am attaching a log file that shows the problem.

Possibly this regression occurred due to the fix for bug 415988.  If you look at my debug log, you will see that all of the JS components are handled as "leftovers" (presumably because the JS loader is not there the first time registration is attempted).  Since the leftovers array is processed in reverse order, it causes registration to occur in the wrong order.

Comment 1

[Kathleen :Brade]

Search for MYEXT in the log file to quickly locate the relevant portion of the log.

Comment 2

[Benjamin Smedberg]

Interesting. Does this same problem occur in libxul or static builds? I think a simple solution might be to process the deferred components in forward order, but I want to make sure that in libxul builds we don't go into the slow-deferred codepath for JS components.

Comment 3

[Mark Smith [:mcs]]

I tried a libxul build and did not see much difference (all of the JS modules are still handled as leftovers).  What do you mean by "the slow-deferred codepath for JS components?"

Comment 4

[Benjamin Smedberg]

The design goal was that binary and JS components would be loaded on the first pass. If an extension was enabled that provided another component loader (e.g. python or java) those would go in the deferred list and be registered later. If JS components are ending up in the deferred list we should fix that. The deferred list uses extra allocations and would be slower than registering immediately.

Comment 5

[Benjamin Smedberg]

Attachment: Re-get component loaders after loading static modules, rev. 1

This should fix both problems. At least in libxul and static builds, we should now correctly load JS components the first time around, instead of sticking them all in the leftover list.

And, this processes the leftover list in forwards order instead of backwards order, so that components which end up there still have a hope of being in a sane order.

Comment 6

[Benjamin Smedberg]

This is a regression against 1.8 which makes it a lot harder for extensions to insert themself into the app... I think it should block especially since the fix is pretty trivial.

Comment 7

[Reed Loden [:reed]]

I think dougt only watches his [e-mail] account. He really should
consolidate!

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 303812 [details] [diff] [review]
Re-get component loaders after loading static modules, rev. 1

we should move the aLeftovers.Count() outside the loop.  (unless inlining will get rid of the function call)

So, we call GetAllLoaders() so that we find any module-loader's that might be in static module.

It seams that:
a) the comment should apply to any module loader, and not specifically call out JS or use JS as an example:

in static and libxul builds, we might find a module loader in the static module.  Set it up now, so that those components don't go into the leftovers list.

b) it seams sort of terrible that every time we go through autoregistration we have to worry about the mStaticModuleLoader.  It seams that this should be done at InitXPCOM() instead of AutoRegister()

Fix the loop, and comment.  Let me know about the AutoRegister() having to worry about the static module.

Comment 9

[Benjamin Smedberg]

The only way to move aLeftovers.Count() outside the loop is if you manually decrement it when you successfully register a leftover component, and I really don't think the extra overhead of keeping the two in sync is worthwhile.

The JS component loader is the only loader in the static component list because that's the only one we ship. Any others (python/java) would be shipped as extra components/extensions.

Comment 10

[Doug Turner (:dougt)]

this code doesn't know what we ship;  I can build python into the static module, right?

Comment 11

[Mark Smith [:mcs]]

Comment on attachment 303812 [details] [diff] [review]
Re-get component loaders after loading static modules, rev. 1

>-    for (PRInt32 i = aLeftovers.Count() - 1; i >= 0; --i) {
>+    for (PRInt32 i = 0; i < aLeftovers.Count(); ++i) {
>         nsresult rv = AutoRegisterComponent(aLeftovers[i], aDeferred,
>                                             minLoader);
>         if (NS_SUCCEEDED(rv))
>             aLeftovers.RemoveObjectAt(i);
>     }

Won't it cause problems to use a forward enumerating for loop in this case (since RemoveObjectAt() is called inside the loop)?

Comment 12

[Benjamin Smedberg]

Yes, I saw that today... I'll post a new patch Tuesday.

Comment 13

[Benjamin Smedberg]

Attachment: Re-get component loaders after loading static modules, rev. 2

Comment 14

[Benjamin Smedberg]

Fixed on trunk for beta4.

Comment 15

[Kathleen :Brade]

Reopening bug.  We encountered this same problem with a deferred component.
Proposed patch coming.

Comment 16

[Kathleen :Brade]

Attachment: proposed fix: preserve order when processing deferred components

Comment 17

[Benjamin Smedberg]

Brade, which deferred component? AFAIK we don't use deferral in the core any more, so talk of deferred components makes me nervous.

Comment 18

[Kathleen :Brade]

I ran into it with nsHelperAppDlg.js.in.

I see at least four occurrences that matter:
http://lxr.mozilla.org/seamonkey/search?string=FACTORY_REGISTER_AGAIN

Additionally, I'd fix nsSample.js in case people start with that file.

Comment 19

[Benjamin Smedberg]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

Please file a followup bug on that list of components: I don't think any of them need deferring, and it will just slow us down a little or maybe more than a little.

Comment 20

[Kathleen :Brade]

Bug #421286 was filed to address current deferred components.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

Do we still need dougt's review here? If so, please renom once it's given. If not, please clear that review flag and renom.

Comment 22

[Benjamin Smedberg]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

no, his review is not necessary

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

a1.9=beltzner

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

r- here: I want a test case before this goes in.  Too much untested behaviour being depended on (apparently!) by external code, need to stop it somewhere and I'm saying it's here!

Hope we don't find out that some other extension was depending on the old order, as Brade's depends on the new order... :/

Comment 25

[Reed Loden [:reed]]

Comment #24 says this needs a test before it can land. This also isn't a blocker, so it needs approval before it can land.

Comment 26

[Kathleen :Brade]

Attachment: test case (part 1 of 2)

Comment 27

[Kathleen :Brade]

Attachment: test case (part 2 of 2 - JS components)

Comment 28

[Benjamin Smedberg]

Comment on attachment 311785 [details] [diff] [review]
test case (part 1 of 2)

Woohoo!

Comment 29

[Benjamin Smedberg]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

Re-requesting approval with the unit-tests below.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307261 [details] [diff] [review]
proposed fix: preserve order when processing deferred components

a=beltzner

Comment 31

[Kathleen :Brade]

Attachment: Revised patch for xpcom/tests/Makefile.in (Windows path)

Thanks for the reviews/approval.  I landed the patch and new tests.  When running the tests on Windows, I noticed that the Makefile was not passing a dos/windows-style path.  This patch replaces the original Makefile diffs and fixes the tests on Windows.

Comment 32

[Kathleen :Brade]

Attachment: test case as checked in (part 1 of 2)

To fix a compile error in non-libXUL builds, I had to remove a #include "nsStringAPI.h" line from TestRegistrationOrder.cpp.  I am attaching this patch to record here exactly what I checked in.