Closed Bug 597809 Opened 10 years ago Closed 9 years ago

javaScript-global-constructor does not work

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: matej.spiller, Assigned: mounir)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Component: Developer Tools → XPCOM
Product: Firefox → Core
QA Contact: developer.tools → xpcom
Version: unspecified → Trunk
Benhjamin, are we registering this category too late or too early or something?
blocking2.0: --- → ?
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.
Status: UNCONFIRMED → NEW
Component: XPCOM → DOM: Mozilla Extensions
Ever confirmed: true
QA Contact: xpcom → general
Assignee: nobody → mounir.lamouri
blocking2.0: ? → betaN+
Blocks: 600460
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
This patch makes nsScriptNameSpaceManager an observer of the category manager.
Attachment #479296 - Flags: review?(peterv)
Whiteboard: [needs review]
Comment on attachment 479296 [details] [diff] [review]
Patch v1

Wouldn't it make more sense to use a weak reference instead of obsering shutdown?
(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?
Make nsScriptNameSpaceManager implement nsSupportsWeakReference and pass true to the call of AddObserver for NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID.
Attached patch Patch v2Splinter Review
This is making nsScriptNameSpaceManager using nsSupportsWeakReference and no longer observing shutdown.
Attachment #479296 - Attachment is obsolete: true
Attachment #490572 - Flags: review?(peterv)
Attachment #479296 - Flags: review?(peterv)
Attached patch Inter diff (obsolete) — Splinter Review
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.
Attachment #490572 - Flags: review?(peterv) → review+
(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.
Whiteboard: [needs review]
Attachment #490573 - Attachment is obsolete: true
Pushed:
http://hg.mozilla.org/mozilla-central/rev/dff259d7b189
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
> > >+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.
(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.
Depends on: 619230
Depends on: 621515
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.