Closed
Bug 285211
Opened 20 years ago
Closed 18 years ago
xpcom component registration picking up the wrong copies of components (e.g,., after viewing about:plugins)
Categories
(Camino Graveyard :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.5
People
(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)
Details
(Keywords: fixed1.8.0.13, fixed1.8.1.4)
Attachments
(3 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
"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•20 years ago
|
||
Using 2005032408 (v0.8+):
Same thing happens. I got the same warning.
Reporter | ||
Comment 2•20 years ago
|
||
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"
Reporter | ||
Comment 3•19 years ago
|
||
This appears to be WFM these days.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → WORKSFORME
Comment 4•19 years ago
|
||
this just happened to me with 1.0b2.
Reporter | ||
Comment 5•19 years ago
|
||
Hrm, I just tried 1.0b2, too, and still WFM.
Reporter | ||
Comment 6•19 years ago
|
||
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)
Reporter | ||
Comment 7•19 years ago
|
||
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
Reporter | ||
Comment 8•19 years ago
|
||
Reporter | ||
Comment 9•19 years ago
|
||
Comment 10•19 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•19 years ago
|
||
I can't reproduce this. Does it depend on what plugins you have installed?
Comment 12•19 years ago
|
||
i don't have anything weird installed.
- flash
- java
- shockwave
- WMP
- quicktime
- adobe pdf viewer
- drm plugin (part of wmp, it seems)
Reporter | ||
Comment 13•19 years ago
|
||
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•19 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.
Reporter | ||
Comment 15•19 years ago
|
||
I can repro with just the JEP and the DefaultPlugin (i.e., just what's shipped with the app).
Comment 16•19 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•19 years ago
|
||
We probably need to add an observer for NS_XPCOM_AUTOREGISTRATION_OBSERVER_ID, "end" and re-register our components there.
Reporter | ||
Updated•19 years ago
|
Assignee: mikepinkerton → nobody
Status: REOPENED → NEW
QA Contact: general
Target Milestone: --- → Camino2.0
Comment 18•19 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.
Reporter | ||
Updated•19 years ago
|
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•19 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•18 years ago
|
||
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.
Comment 21•18 years ago
|
||
(In reply to comment #20)
> Maybe this is associated with this bug.
It isn't.
Reporter | ||
Comment 22•18 years ago
|
||
Per comment 18, this should block 1.1. Who's up for taking it?
Flags: camino1.1+
Reporter | ||
Comment 23•18 years ago
|
||
Stuart, is this something you can fix (comment 17 is the desired approach)?
Flags: camino1.1a1?
Reporter | ||
Updated•18 years ago
|
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
Updated•18 years ago
|
Flags: camino1.1a1? → camino1.1a1-
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → stuart.morgan
Assignee | ||
Comment 24•18 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•18 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•18 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).
Assignee | ||
Comment 28•18 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.
Reporter | ||
Comment 29•18 years ago
|
||
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•18 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.
Reporter | ||
Comment 31•18 years ago
|
||
Minusing for b1 per comment 30.
Flags: camino1.1b1? → camino1.1b1-
Assignee | ||
Comment 32•18 years ago
|
||
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)
Comment 33•18 years ago
|
||
you don't need NS_INIT_ISUPPORTS() anymore
Comment 34•18 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•18 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•18 years ago
|
||
Removes NS_INIT_ISUPPORTS
Attachment #259611 -
Flags: superreview?(mikepinkerton)
Comment 37•18 years ago
|
||
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
Updated•18 years ago
|
Attachment #259611 -
Flags: superreview?(mikepinkerton) → superreview+
Assignee | ||
Comment 38•18 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•18 years ago
|
||
Checked in with do_GetService change on trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Closed: 19 years ago → 18 years ago
Keywords: fixed1.8.1.4
Resolution: --- → FIXED
Reporter | ||
Comment 41•18 years ago
|
||
Approving for 1.0.5.
Flags: camino1.0.5? → camino1.0.5+
Whiteboard: [needs checkin 1.8.0]
Assignee | ||
Comment 42•17 years ago
|
||
As checked in on MOZILLA_1_8_0_BRANCH
Attachment #259552 -
Attachment is obsolete: true
Attachment #259611 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs checkin 1.8.0]
Reporter | ||
Updated•17 years ago
|
Keywords: fixed1.8.0.12
Updated•17 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.
Description
•