89528 - access-proxy always active in static build

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[cls]

In a static build, I'm noticing lots of messages to the console about various
events that occur in the build:
==== keyup event occurred ====
==== click event occurred ====
** nsAccessProxy ** OnSecurityChange ** 

Outside of the fact that those printfs should be in a DEBUG ifdef, I think we
have a problem here as I don't see those same printfs in my dynamic build.  Some
how, we are doing more than just registering the components in the static build;
we are actually traversing this module's code path.  This seems like it could be
a potential performance problem.

Waterson, you mentioned that possibly the use of a static constructor could
cause this problem.  

In the use of NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR in 
http://lxr.mozilla.org/seamonkey/source/extensions/access-builtin/accessproxy/nsAccessProxyRegistration.cpp#76
, is it possible that the call to ::GetInstance which allocates a new
nsAccessProxy object causes the module to become active?  Is there any way to
work around this?

Comment 1

[Chris Waterson]

It looks like the macro in question is used like this:

NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsAccessProxy,nsAccessProxy::GetInstance);

It's defined here:

http://lxr.mozilla.org/mozilla/source/xpcom/components/nsIGenericFactory.h#217

As far as I can tell, what it does is set up a routine that's palatable to the
generic factory magic that will call |nsAccessProxy::GetInstance()| to acquire
the single nsAccessProxy instance. |nsAccessProxy::GetInstance()| is defined here:

http://lxr.mozilla.org/mozilla/source/extensions/access-builtin/accessproxy/nsAccessProxy.cpp#77

It looks kosher to me; specifically, it checks a static |mInstance| pointer
variable, and if null, creates a new one. (This _might_ be a leak, though...can
somebody convince me that this is actually destroyed at some point?)

Comment 2

[cls]

When does nsAccessProxy::Initialize() get called in a dynamic build and how is
that different from the static build?  I verified that if I comment out the
registration of nsAccessProxy as a webshell listener,
http://lxr.mozilla.org/seamonkey/source/extensions/access-builtin/accessproxy/nsAccessProxy.cpp#172
, then the nsAccessProxy event handler routines are never used and the messages
don't show up.

Comment 3

[cls]

Ok, so after pulling Waterson away from his Big Brother premiere tonight, he was
able to narrow down the actual problem while I meandered around aimlessly:

If you're going to copy-n-paste the sample code, make sure that you CHANGE THE
CONTRACTID!  Dumping the component registry showed that there were two
components using the CID that accessproxy was claiming to be registered using. 
The other component was nsSample from mozilla/xpcom/sample!  The reason that the
conflict does not occur in the static build is that the sample component is not
in the list of components to be linked into the final executable.

So once access-proxy is given it's own unique CID, everyone who builds using
--with-extensions will also start getting the debug spew caused by this module
(for which I just opened bug 89540).

Comment 4

[Aaron Leventhal]

Attachment: Also fixes 89540 - accessproxy must not print to console

Comment 5

[Aaron Leventhal]

Chris W,

void nsAccessProxy::ReleaseInstance()
{
  NS_IF_RELEASE(nsAccessProxy::mInstance);
}

This is called when I close my last Mozilla Window.
Verified with debugger.

Someone want to r= and sr= ?

Comment 6

[cls]

I can't vouch for the other changes but the commenting of the
NS_DEBUG_ACCESS_BUILTIN define and the changing of the CID look ok to me. r=cls
on those parts of the patch.

Comment 7

[Chris Waterson]

You leak the character pointers from CopyCharPref(). Use nsXPIDLCString there.

I'm sure that we're all thankful of this, but is the comment really necessary? 
;-)

+  NS_IMETHOD HandleEvent(nsIDOMEvent *event);  // This compiles on Windows

Looks good otherwise. r/sr=waterson

Comment 8

[Aaron Leventhal]

checked in

Comment 9

[Mike Kaply [:mkaply]]

Every ports tinderbox is lighting up right now.

Something related to this bug.

Comment 10

[Mike Kaply [:mkaply]]

I believe it was a stray semicolon in the idl file.

Fix checked in