597809 - javaScript-global-constructor does not work

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Matej Spiller-Muys]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.62 Safari/534.3
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

According to Gecko2 XPCom changes I have updated my C++ xpcom component.

Calling it from javascript the constructor is not registered.

My CategoryEntry snippet:

static const mozilla::Module::CategoryEntry kSampleCategories[] = {
    { "JavaScript-global-property", "MyComponent", MY_COMPONENT_CONTRACTID },
    { "Javascript-global-constructor", "MyComponent", MY_COMPONENT_CONTRACTID },
    { NULL }
};

My updated component does work in FF 3.6 (suing NS_IMPL_MOZILLA192_NSGETMODULE(&kSampleModule)).
However the constructor in javascript does not work. I have tried setting only constructor, but the problem remains the same.

Sample html code:
netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

//this works
var sample = Components.classes["@company/MyComponent;1.0"].createInstance();
sample = sample.QueryInterface(Components.interfaces.nsIMyComponent);
alert(sample.getId());

//this also works
alert(MyComponent.getId());

//this no longer works (works only in FF 3.6, not in 4.0b6)
alert(new MyComponent().getId());

Current workaround is to use property, and create constructor method inside that property:
MyComponent.createMyComponentInstance();


Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Benhjamin, are we registering this category too late or too early or something?

Comment 2

[Benjamin Smedberg]

Well, the in-browser case works:

http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/BrowserFeeds.manifest#13

This appears to be filled here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptNameSpaceManager.cpp#461

And doesn't use a category observer, so if nsScriptNameSpaceManager::Init happens before extensions get registered (which is likely), then yes, there's an ordering problem that should be solved in the script namespace manager.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This patch makes nsScriptNameSpaceManager an observer of the category manager.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 479296 [details] [diff] [review]
Patch v1

Wouldn't it make more sense to use a weak reference instead of obsering shutdown?

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to comment #4)
> Comment on attachment 479296 [details] [diff] [review]
> Patch v1
> 
> Wouldn't it make more sense to use a weak reference instead of obsering
> shutdown?

I'm not following. What could be a weak reference?

Comment 6

[Peter Van der Beken [:peterv]]

Make nsScriptNameSpaceManager implement nsSupportsWeakReference and pass true to the call of AddObserver for NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

This is making nsScriptNameSpaceManager using nsSupportsWeakReference and no longer observing shutdown.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 490572 [details] [diff] [review]
Patch v2

>diff --git a/dom/base/nsScriptNameSpaceManager.cpp b/dom/base/nsScriptNameSpaceManager.cpp

>+    // We are observing the category manager if initialized.
>+    nsCOMPtr<nsIObserverService> obsSvc =
>+      do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
>+
>+    if (obsSvc) {
>+      obsSvc->RemoveObserver(this, NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID);
>+    }
>+

You could just remove this, no?

>+nsScriptNameSpaceManager::AddCategoryEntryToHash(nsICategoryManager* aCategoryManager,

>+  // Get the type from the category name.
>+  // NOTE: we could have pass the type in FillHash() and guessing it in

... passed ..., ... guessed ...

>+  // Observe() but this way, we have only one place to update and that's
>+  // not performance sensitive.

... we only have one place to update and this is ...

>+  // Copy CID onto the stack, so we can free it right away and avoid having
>+  // to add cleanup code at every exit point from this loop/function.

s/loop//

>diff --git a/dom/base/nsScriptNameSpaceManager.h b/dom/base/nsScriptNameSpaceManager.h

>+class nsScriptNameSpaceManager : public nsIObserver
>+                               , public nsSupportsWeakReference

Comma at the end of previous line.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> >diff --git a/dom/base/nsScriptNameSpaceManager.cpp b/dom/base/nsScriptNameSpaceManager.cpp
> 
> >+    // We are observing the category manager if initialized.
> >+    nsCOMPtr<nsIObserverService> obsSvc =
> >+      do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> >+
> >+    if (obsSvc) {
> >+      obsSvc->RemoveObserver(this, NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID);
> >+    }
> >+
> 
> You could just remove this, no?

Indeed. I wasn't able to see when/how/if the observer was removed so I've add that to be sure but I've found that now.

> >diff --git a/dom/base/nsScriptNameSpaceManager.h b/dom/base/nsScriptNameSpaceManager.h
> 
> >+class nsScriptNameSpaceManager : public nsIObserver
> >+                               , public nsSupportsWeakReference
> 
> Comma at the end of previous line.

According to the coding style, this seems correct.

Comment 11

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/dff259d7b189

Comment 12

[Peter Van der Beken [:peterv]]

> > >+class nsScriptNameSpaceManager : public nsIObserver
> > >+                               , public nsSupportsWeakReference
> > 
> > Comma at the end of previous line.
> 
> According to the coding style, this seems correct.

Huh, can you point me to a file in dom/base that does it? AFAIK in dom/ we've always done operators at end of line.

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> > > >+class nsScriptNameSpaceManager : public nsIObserver
> > > >+                               , public nsSupportsWeakReference
> > > 
> > > Comma at the end of previous line.
> > 
> > According to the coding style, this seems correct.
> 
> Huh, can you point me to a file in dom/base that does it? AFAIK in dom/ we've
> always done operators at end of line.

Actually, I was referring to this document: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

I forget that some modules have specific coding style... I will fix that.

Comment 14

[Mounir Lamouri (:mounir)]

Fixed: http://hg.mozilla.org/mozilla-central/rev/87c48eea3c6c