xpcom component registration picking up the wrong copies of components (e.g,., after viewing about:plugins)

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
General
P2
major
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Stuart Morgan)

Tracking

({fixed1.8.0.13, fixed1.8.1.4})

Details

Attachments

(3 attachments, 3 obsolete attachments)

"Warn on enter a secure site" has apparently been permanently disabled in Camino
(setting the security.warn_entering_secure* prefs has no effect); however,
viewing about:plugins re-enables the warning (for that browser session).  This
is presumably a regression from the first appearance of a working about:plugins,
but I first noticed it in 2005030608 (v0.8+).

At the moment that also makes Camino subject to bug 284357.

Comment 1

13 years ago
Using 2005032408 (v0.8+):

Same thing happens. I got the same warning.
It also re-enables the "warn on submitting insecure form" dialogue.  I'm
guessing at this point that there are other dialogues that have been hard-coded
off in Camino that viewing about:plugins is going to re-enable.  Bizarre.
Summary: viewing about:plugins reenables "warn on entering secure site" → viewing about:plugins reenables "warn on entering secure site", "warn submitting insecure form"
This appears to be WFM these days.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WORKSFORME
this just happened to me with 1.0b2.
Hrm, I just tried 1.0b2, too, and still WFM.
Reopening; Mike isn't crazy, but maybe I was before.

Visiting about:plugins *does* re-enable "warn on entering secure site" and "warn on submitting an insecure form" in 2006012904 (v1.0b2+)

("Warn on submitting an insecure form from a secure website" seems to be on permanantly, unrelated to this regression)
Status: RESOLVED → REOPENED
Keywords: regression
Resolution: WORKSFORME → ---
I was so crazy...this was broken the day in July I claimed it was WFM.  In fact, it was broken back as far as 0.8.4...we just didn't notice since about:plugins displayed nothing and wasn't a menu item until Mar 2005.

Setting security.warn_entering_secure to false seems to prevent that behavior, so for that pref, adding it to all-camino.js might be a viable quick work-around.

Setting security.warn_submit_insecure to false also seems to prevent that sheet from displaying.

Still, visiting about:plugins manages to distort/change the "warn on submitting an insecure form from a secure website" sheet.
Keywords: regression
Created attachment 210141 [details]
Normal "warn when submitting form from secure->insecure" dialogue
Created attachment 210142 [details]
Same dialogue, after having visited about:plugins

Comment 10

12 years ago
Weird; does it happen in FF too?

Loading about:plugins causes XPCOM to re-register components, so maybe some component is resetting those defaults at that time? 

Comment 11

12 years ago
I can't reproduce this. Does it depend on what plugins you have installed?
i don't have anything weird installed.

- flash
- java
- shockwave
- WMP
- quicktime
- adobe pdf viewer
- drm plugin (part of wmp, it seems)
No, it doesn't happen in Fx, but it doesn't disable those dialogues in code, either (or at least that looks like what we're doing here:  
http://landfill.mozilla.org/mxr-test/mozilla/source/camino/src/browser/SecurityDialogs.mm#902
see also lines 916 and 962).

Fx displays *all* of the security dialogues on first encounter (and the "always warn me" check box defaults to unchecked in every single one, not just the stupid ones!), and it honors the prefs thereafter.  There are additional prefs to cover the checkboxes (a .show_once partner to each warn.foo).

Fx also doesn't seem to have a custom "secure site->insecure form" dialogue; it has the same text as the second image I posted above, regardless of whether Fx has visited about:plugins or not.

Comment 14

12 years ago
Put this patch in MainController:

Index: MainController.mm
===================================================================
RCS file: /cvsroot/mozilla/camino/src/application/MainController.mm,v
retrieving revision 1.170
diff -u -r1.170 MainController.mm
--- MainController.mm	29 Jan 2006 04:56:29 -0000	1.170
+++ MainController.mm	30 Jan 2006 16:32:41 -0000
@@ -261,6 +261,12 @@
   // listen for the Show Certificates notification (which is send from the Security prefs panel)
   [notificationCenter addObserver:self selector:@selector(showCertificatesNotification:) name:@"ShowCertificatesNotification" object:nil];
   
+// testing
+  [notificationCenter addObserver:self selector:@selector(prefChanged:) name:kPrefChangedNotificationName object:nil];
+  [[PreferenceManager sharedInstance] addObserver:self forPref:"security.warn_entering_secure"];
+// testing
+
+
   [self setupStartpage];
 
   // Initialize offline mode.
@@ -296,6 +302,12 @@
   [self performSelector:@selector(checkDefaultBrowser) withObject:nil afterDelay:2.0f];
 }
 
+
+- (void)prefChanged:(NSNotification*)inNote
+{
+  NSLog(@"Pref changed");
+}
+
 - (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication *)sender
 {
   ProgressDlgController* progressWindowController = [ProgressDlgController existingSharedDownloadController];


and break in the debugger in the prefChanged:. Then go to about:plugins and see who's setting that.
I can repro with just the JEP and the DefaultPlugin (i.e., just what's shipped with the app).

Comment 16

12 years ago
This has the same root cause as bug 278928. The issue is that loading about:plugins causes XPCOM to do autoregistration. This allows dylibs to override our internal components that we hand-registered with NS_NewGenericFactory()/RegisterFactory().

So after loading about:plugins, you're getting NSS's  NS_SECURITYWARNINGDIALOGS implementation, and not Camino's.

dougt: is there a way we can get notified when autoreg happens, and re-register our internal components last?

Comment 17

12 years ago
We probably need to add an observer for NS_XPCOM_AUTOREGISTRATION_OBSERVER_ID, "end"  and re-register our components there.
Assignee: mikepinkerton → nobody
Status: REOPENED → NEW
QA Contact: general
Target Milestone: --- → Camino2.0

Comment 18

12 years ago
I think the underlying issue (xpcom component registration picking up the wrong copies of components) is serious enough to warrant fixing before 2.0. As it is now, Camino is very susceptible to changes in core components. I suggest an approach to fix this in comment 17.
Severity: minor → major
Priority: -- → P2
Summary: viewing about:plugins reenables "warn on entering secure site", "warn submitting insecure form" → xpcom component registration picking up the wrong copies of components (e.g,., after viewing about:plugins)
Target Milestone: Camino2.0 → Camino1.1

Comment 19

12 years ago
#17 seams like the easiest thing to do.  all embedding applications are going to have this problem if they override components:

http://lxr.mozilla.org/mozilla1.8/ident?i=RegisterFactory

Comment 20

12 years ago
Created attachment 228791 [details]
Testcase idl file. Compile and import in simple xul app (not this bug)

Make a simple .xul window

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" width="640" height="480">
    <button flex="1" label="click" oncommand="alert(Components.interfaces.nsIExeDataExtractor)"/>
</window>

The nsIExeDataExtractor doesn't get through to JS. Clicking the button gives undefined, works ok in ff though.

Maybe this is associated with this bug. Tested on two PCs happened when upgrading my app, seemed to be getting the older interfaces but not the new ones.
(In reply to comment #20)
> Maybe this is associated with this bug.

It isn't.
Per comment 18, this should block 1.1.  Who's up for taking it?
Flags: camino1.1+
Stuart, is this something you can fix (comment 17 is the desired approach)?
Flags: camino1.1a1?
Attachment #228791 - Attachment description: Testcase idl file. Compile and import in simple xul app → Testcase idl file. Compile and import in simple xul app (not this bug)
Attachment #228791 - Attachment is obsolete: true
Flags: camino1.1a1? → camino1.1a1-
(Assignee)

Updated

12 years ago
Assignee: nobody → stuart.morgan
(Assignee)

Comment 24

12 years ago
Either I'm missing something, or the docs at http://www.mozilla.org/projects/embedding/observer-topics.html are lying.  For xpcom-autoregistration it says "You can register for this event by using the nsIObserver Service", and for xpcom-shutdown it says "You can register for this event either by using the nsIObserver Service or by adding a category entry under the category of the topic name". Yet if I make an nsIObserver subclass and register it with nsIObserverService, it works fine for xpcom-shutdown but never gets called for xpcom-autoregistration (and I'm manually triggering AutoRegister and veryifying it's being called in the debugger).

If I look at NS_ShutdownXPCOM I see that it calls NotifyObservers, whereas AutoRegister instead calls a function which enumerates categories and calls Observe on any that conform to nsIObserver. I'm certainly not familiar with this code, but that sounds completely backwards from what the docs say.

Could someone familiar with this weigh in on what's going on here?
(Assignee)

Comment 25

12 years ago
bsmedberg, josh suggested you'd be a good person to comment on comment 24, and specifically what the right way to listen for xpcom autoregistration would be.

Comment 26

12 years ago
I don't know. registerFactory is obviously a terrible hack, but it seems that you'd have the same problem if you were using the recommended technique for registering static components (passing a static component list to NS_InitXPCOM3).

Comment 27

11 years ago
Should this block beta?

cl
Flags: camino1.1b1?
(Assignee)

Comment 28

11 years ago
I've started experimenting with doing this as a category entry, but it's much more involved and I'm learning as I go, so I haven't figured out if it works yet.

If anyone could point me to existing code that listens for auto-registration and actually works, it would help a lot.
I was able to experience this bug again in yesterday's branch nightly after playing with the disable/enable plugins pref and choosing Installed Plug-Ins a few times....

(Hmm, I thought there was a comment about us not seeing it and not being able to repro when Stuart started working on this...maybe he just mentioned it on irc or at a meeting?)
(Assignee)

Comment 30

11 years ago
Since I still don't know why code that seems like it should work doesn't, I don't think this should hold b1. I'll keep trying for 1.1 final though.
Minusing for b1 per comment 30.
Flags: camino1.1b1? → camino1.1b1-
(Assignee)

Comment 32

11 years ago
Created attachment 259552 [details] [diff] [review]
fix

The category method of listening works fine, so I guess the docs just lie.

Simon, would you be willing to look this over? If not, feel free to just reassign the review to a generic r?
Attachment #259552 - Flags: review?(sfraser_bugs)
you don't need NS_INIT_ISUPPORTS() anymore

Comment 34

11 years ago
Comment on attachment 259552 [details] [diff] [review]
fix

This all looks good. My only question is whether you could simply add your sAutoregistrationListenerComponentInfo to the list of components we already have in AppComponents.mm, and have it registered by the existing CHBrowserService::RegisterAppComponents() code.
Attachment #259552 - Flags: review?(sfraser_bugs) → review+
(Assignee)

Comment 35

11 years ago
I thought about that, but I like to pretend sometimes that this will one day be the framework it was originally intended to be, in which case it would be nice to have the behavior built-in rather than all its clients having to do the right thing.
(Assignee)

Comment 36

11 years ago
Created attachment 259611 [details] [diff] [review]
v2

Removes NS_INIT_ISUPPORTS
Attachment #259611 - Flags: superreview?(mikepinkerton)
Comment on attachment 259611 [details] [diff] [review]
v2

+  // Set it to listen for XPCOM autoregistration.
+  nsCOMPtr<nsICategoryManager> cm = do_GetService(NS_CATEGORYMANAGER_CONTRACTID, &rv);
+  if (NS_SUCCEEDED(rv)) {

usually it's ok to use the non-rv version of this call and just check if the service is NULL. Since you really don't care about *what* failed, just makes it a little cleaner.

sr=pink
Attachment #259611 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 38

11 years ago
That's kind of embarrassing, given that I made the same argument in one of my recent patches and have been thinking about cleaning up our existing do_GetService calls ;)
(Assignee)

Comment 39

11 years ago
Checked in with do_GetService change on trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Last Resolved: 13 years ago11 years ago
Keywords: fixed1.8.1.4
Resolution: --- → FIXED
Should we look at taking this for 1.0.5?
Flags: camino1.0.5?
Approving for 1.0.5.
Flags: camino1.0.5? → camino1.0.5+
Whiteboard: [needs checkin 1.8.0]
(Assignee)

Comment 42

11 years ago
Created attachment 264398 [details] [diff] [review]
v3

As checked in on MOZILLA_1_8_0_BRANCH
Attachment #259552 - Attachment is obsolete: true
Attachment #259611 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Whiteboard: [needs checkin 1.8.0]
Keywords: fixed1.8.0.12

Updated

11 years ago
Keywords: fixed1.8.0.12 → fixed1.8.0.13
You need to log in before you can comment on or make changes to this bug.