Closed Bug 258123 Opened 20 years ago Closed 20 years ago

Cannot register own JavaScript Component with new interface

Categories

(Toolkit :: Add-ons Manager, defect)

1.7 Branch
defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

(Reporter: patrick, Assigned: shaver)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(5 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

With the latest Firefox and Thunderbird builds (Thunderbird >= 20040901), it is
not possible anymore to correctly register a component written in JavaScript
that provides a specific interface not previously known to FF / TB.

Reproducible: Always
Steps to Reproduce:
1. Install the attached XPI
2. Type chrome://enigmail/content/enigmailAbout.xul into the URL bar, or
window.openDialog("chrome://enigmail/content/enigmailAbout.xul") in the
JavaScript  console
Actual Results:  
On Mozilla 1.7 and Thunderbird 0.7, you'll see that the dialog prints success,
i.e. the dummy component is correctly loaded. With the nightly FF and TB, you'll
see an exception saying that Components.interfaces.nsIEnigmail does not exist,
but that's what the XPI should provide.
Attached file XPI showing the bug
Version: unspecified → 1.0 Branch
The attached XPI can be unpacked and built in mozilla/extensions
Maybe this is of some interest:

 If I start with a fresh profile and copy first the components/ files of the
example xpi to the thunderbird components directory, then start thunderbird and
install the xpi through the extension manager, the test case will succeed.

If I just install the .xpi the test-case fails and it is not recoverable by
copying the components/ files of the xpi archive manually to the thunderbird
components directory anymore.

For me this sounds as if components in the
$HOME/.thunderbird/xxx/extensions/xxxx/components are not loaded, but registered
only (since manually copying those files to the global components directory
after that won't help and won't register any new components)
As pointed by alex@elbrus.com, this must be a duplicate of bug 257681.
If you agree, please post the testcase# (157963) there and mark this bug as a
dup of that?
I'm sorry, I searched for such a bug, but couldn't find one.

*** This bug has been marked as a duplicate of 257681 ***
Status: UNCONFIRMED → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
So I think this is correct (it works in my testing with the attached
extension-test, which failed before my patch), because xptiInfoManager goes to
some lengths to only reregister files that it doesn't already have data for,
and additionally to make sure that duplicate interfaces are really identical. 
I've convinced myself that this specific reload case is safe.  (It might well
be safe in the general case, but I only did the mental gymnastics to track this
part, so we should revisit it before, f.e., attempting this during no-restart
extension install.)
Assignee: bugs → shaver
Status: RESOLVED → ASSIGNED
Resolution: DUPLICATE → ---
Comment on attachment 158260 [details] [diff] [review]
expose interface for rebuilding search path

I'd love to hear from the real xptinfo experts, though.

I added this additional interface so that I wouldn't disturb the web services
code, which implements nsIInterfaceInfoManager as well.
Attachment #158260 - Flags: superreview?(jband_mozilla)
Attachment #158260 - Flags: review?(dbradley)
Flags: blocking-aviary1.0PR+
*** Bug 257681 has been marked as a duplicate of this bug. ***
Comment on attachment 158260 [details] [diff] [review]
expose interface for rebuilding search path

This isn't sufficient; the extra dirs are being hidden from xptiinfomanager on
non-registering starts of firefox (not the restart-to-install, but every one
after that) because we don't go through
nsXREDirProvider::RegisterExtraComponents, and mRegisterExtraComponents stays
PR_FALSE.

Now, I'm not sure why we're not just working from the same xpti.dat as before,
but it's probably because the dir list is different.  Alas...

(Also, this patch is in the wrong bug, arguably.)
Attachment #158260 - Attachment is obsolete: true
Attached patch doodling (obsolete) — Splinter Review
I needed shaver's patch after all for various reasons. But the end result is
something that still doesn't work. Things are registered, but the
NS_ERROR_XPC_CI_RETURNED_FAILURE error persists.
Background:

When an extension is installed, the contents of its XPI file are inflated to:
<dir>/extensions/{GUID}/

Shaver proposed the following:

When an extension is installed or enabled, the contents of the extension's
"components" directory are copied from:
<dir>/extensions/{GUID}/ to <dir>/extensions/components/{GUID}

This way, xptiInterfaceInfoManager would only have to know about one components
dir, instead of several.

When an extension is uninstalled or disabled, its {GUID} dir under
<dir>/extensions/components would be removed. 

The reason shaver's patch is still required is that we can't tell the Component
Registry or the xptiInterfaceInfoManager about the new
<dir>/extensions/components dir until *after* the EM has been started and
pending EM uninstall/install operations have been performed, otherwise CompReg
etc may load components in that location and we will be unable to delete them
(because they will be busy). 

So I needed a way to force the xptiInterfaceInfoManager to rebuild its search
path (supplied by shaver's patch). 

The additional code in my patch is related to the fact that
xptiInterfaceInfoManager, despite shaver's strenuous insistence (:p) does *not*
seem to enjoy scanning recursive subdirs for components. I had to teach
xptiInterfaceInfoManager how to look deeper into subdirs and to consider parent
chains when testing for file containment. Finally, the remainder of my patch (in
and around nsExtensionManager.js.in, nsAppRunner.cpp and nsXREDirProvider.cpp)
is to do with getting rid of components.ini and simply using the "components set
changed" flag as a trigger to call autoregister. 

So, a lot of code. But does it work? No. The Enigmail example throws the
exception shaver was seeing earlier:

JavaScript error:
 line 0: uncaught exception: [Exception... "Component returned failure code: 0x8
0570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE) [nsIJSCID.createInstance]"  nsresult:
 "0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE)"  location: "JS frame :: chrome:
//enigmail/content/enigmailAbout.xul :: enigAboutLoad :: line 29"  data: no]

possibly because without knowing it, my patch has recreated exactly the same
scenario that shaver created earlier, just in a much more complicated way. I
have to say, if it's not going to work, I prefer the simplicity of the original
setup ;-) 

Now, I could be wrong and maybe there's something simple wrong with what I have
that'd make it work. I now hand the batton to shaver to carry for the early
morning, while I slumber. 
I went deeper here, and tightened up the logic for search-path changes a bit,
including support for skipping newly-missing dirs, which helps here.  The dirs
need to be missing at the end, because I'm not doing dir-index remapping work
here, so I moved the plugins dir _before_ extensions.  I don't think it's a
problem, but you can try to convince me otherwise.

One thing that made me a little nervous is appending the empty xptiFile, but I
don't think we'll ever be asked for it, because the interface entries will be
missing, so nothing will point to that index.

This makes the test work for me, other than the fact that .value doesn't
actually update.  I can print and operate on the returned |a| fine, though, so
I think there's something else at play here.  (Also, moving the assignment to
before the Components poking doesn't change things, so I call "SEP".)
Attachment #158296 - Attachment is obsolete: true
Attachment #158260 - Flags: superreview?(jband_mozilla)
Attachment #158260 - Flags: review?(dbradley)
So it turns out that this bug was caused by:

1.27.2.1.4.36 <ben@bengoodger.com> 2004-08-31 22:01
Fix numerous Extension Manager/Update bugs, mostly 257304 but also: 247322
252543 256930 256960 256961 257045 257047 257179
reviews pending.

Which moved the RegisterExtraComponents call down to _below_ the XPCOM
autoregistration, which then meant that we violated the "only start xptIIM once
all the components are visible" tenet that informed the original design of this
restart mess.

Darin and I would like to explore more why that change was made, and if there
are other options that would avoid the need for this patch.

(I do think that the patch moves the IIM in the right direction, and would be
interested in taking some variant of it on the trunk.)
> (I do think that the patch moves the IIM in the right direction, and would be
> interested in taking some variant of it on the trunk.)

Absolutely!  And, maybe for the trunk we can make it so that
AutoRegisterInterfaces rebuilds the path set automagically, thus avoiding the
need for the additional interface.
Attached patch patch (obsolete) — Splinter Review
OK - this fixes all the bugs.

This patch excludes extension components dirs from the components list while
their "toBeInstalled" flag is set - i.e. while they're being installed or
upgraded - so there will be no components loaded if we need to overwrite. 

Backs out change to the earlier bug (to nsAppRunner.cpp/nsXREDirProvider.cpp)
about not being able to upgrade components.
Attached patch patchSplinter Review
Make XREDirProvider also not tell far-fetched adventure stories to the
component manager about what components are available, now that we've
guaranteed the component list in components.ini will be sane.
Attachment #158378 - Attachment is obsolete: true
Comment on attachment 158379 [details] [diff] [review]
patch

I like it.  Best so far, for sure.

Darin will like it too, I bet.
Attachment #158379 - Flags: superreview?(darin)
Attachment #158379 - Flags: review+
Comment on attachment 158379 [details] [diff] [review]
patch

Looks good to me, let's get this done!
Attachment #158379 - Flags: superreview?(darin) → superreview+
Ditto
Patch checked in. Shaver, you probably want to take your patch to a separate bug
if you want to pursue it on the trunk. 
Status: ASSIGNED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Keywords: fixed-aviary1.0
Comment on attachment 158945 [details] [diff] [review]
porting the changes in toolkit/xre to the trunk

this doesn't completely port ben's patch, but i believe this bug doesn't exist
on the trunk.  i'm only doing this patch to ease merging aviary onto the trunk
*a little bit*
Attachment #158945 - Flags: review?(bsmedberg)
Attachment #158945 - Flags: review?(bsmedberg) → review+
attachment 158945 [details] [diff] [review] has been checked in on the trunk
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: