158801 - factor security dialogs out of CocoaBrowserService

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Brian Ryner (not reading)]

nsCocoaBrowserService implements 9 interfaces.  This is a lot, and it makes the
source file quite large (and hard to navigate).  One easy win I see is to move
the security dialogs implementation off into another singleton, as I don't see a
compelling reason for it to be on nsCocoaBrowserService.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

pinkerton, what do you think?  Can you review?

Comment 2

[Greg K.]

Whoops, removing patch. I imagine Brian has check-in privs.

Comment 3

[Mike Pinkerton (not reading bugmail)]

+  rv = cr->RegisterFactory(kBadCertHandlerCID, "Bad Cert Handler",
+                           NS_NSSDIALOGS_CONTRACTID, sSingleton);

rather than registering the cocoaBrowserFactory singleton as the implementor and
delegating to the dialog singleton, why not just make the dialog singleton be
the factory? is it just the extra COM factor machinery you want to avoid? I just
think it'll save one step of indirection, and save one step in readability, to
be the factory.

otherwise r=pink.

Comment 4

[Brian Ryner (not reading)]

Attachment: patch #2

addressing pink's comments

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 92449 [details] [diff] [review]
patch #2

looks good r=pink

Comment 6

[Simon Fraser [no longer active]]

+class SecurityDialogs : public nsIBadCertListener,
+                        public nsISecurityWarningDialogs,
+                        public nsINSSDialogs,
+                        public nsIFactory

Would it be better to implement the factory in a separate class? It would make
it easier for other cocoa embedders to use a different impl.

+  // HACK: there is no way to get which window this is for from the API. The
+  // security team in mozilla just cheats and assumes the frontmost window so
+  // that's what we'll do. Yes, it's wrong. Yes, it's skanky. Oh well.

Is there a bug on this? Seems that it might be the source of some potential for
spoofing attacks.



+SecurityDialogs* nsCocoaBrowserService::sSecurityDialogs = nsnull;
...
+  sSecurityDialogs = new SecurityDialogs();
+  if (!sSecurityDialogs)
+    return NS_ERROR_OUT_OF_MEMORY;
+  NS_ADDREF(sSecurityDialogs);
+
+  rv = cr->RegisterFactory(kBadCertHandlerCID, "Bad Cert Handler",
+                           NS_NSSDIALOGS_CONTRACTID, sSecurityDialogs);

If the factory was a separate class, you could just allocate a factory object,
and pass its ownership to the component registry. Then there's no need for
static objects. It seems a bit cleaner.

Comment 7

[Simon Fraser [no longer active]]

I realise that having a separate factory that returns SecurityDialogs instances
means that we'll then make > 1 (each with a copy of the string bundle). Not sure
if this is an issue.

Comment 8

[Brian Ryner (not reading)]

> Would it be better to implement the factory in a separate class? It would make
it easier for other cocoa embedders to use a different impl.

It could be, sure, but I'd want to keep it in the same file.  The chimera/
directory is already huge, let's not add an additional source file for each
xpcom component we implement.

> Is there a bug on this? Seems that it might be the source of some potential
for spoofing attacks.

I didn't add this, I'm just moving code.  Ask pinkerton.

> If the factory was a separate class, you could just allocate a factory object,
and pass its ownership to the component registry. Then there's no need for
static objects. It seems a bit cleaner.

Sure.

> I realise that having a separate factory that returns SecurityDialogs
instances means that we'll then make > 1 (each with a copy of the string
bundle). Not sure if this is an issue.

No, we won't, because nsINSSDialogs is used as a service, so the service manager
will hold onto the first instance we create and return it when the service is
requested again.

Comment 9

[Brian Ryner (not reading)]

Attachment: patch #3

What I've done here is make use of nsIGenericFactory, so we don't have to mess
with a static singleton (doing the singleton thing ourselves is totally
unnecessary anyway, since nsINSSDialogs is a service).	I've provided a way to
easily add new application-provided gecko components as well, simply add them
to AppComponents.cpp.

Comment 10

[Conrad Carlen (not reading bugmail)]

This technique is very nice & clean:

+  nsModuleComponentInfo* componentInfo = GetAppModuleComponentInfo();
+  PRInt32 numComponents = sizeof(*componentInfo) / sizeof(componentInfo[0]);
+  for (int i = 0; i < numComponents; ++i) {

except, isn't numComponents always going to evaluate to 1, since the compiler
can't see the declaration of the array and it just appears to be a pointer? 

Comment 11

[Mike Pinkerton (not reading bugmail)]

> Is there a bug on this? Seems that it might be the source of some potential
for spoofing attacks.

I swear I filed a bug, but I can't seem to find it. I remember talking to the
security guys about this and they weren't too concerned.

Comment 12

[Brian Ryner (not reading)]

Attachment: patch v3.1

Fixed the array sizeof() problem, and merged to the trunk.

Comment 13

[Brian Ryner (not reading)]

So, once this is checked in, I'd like to move the rest of the components we
register to the same mechanism.  That would be the prompt service, the helper
app launcher dialog component, and the download component.  Then we should be
able to make nsCocoaBrowserService no longer be an nsIFactory, as well.

Comment 14

[Simon Fraser [no longer active]]

Comments:

+static nsModuleComponentInfo components[] = {

Make this 'const'

+nsModuleComponentInfo* GetAppModuleComponentInfo(int* numComponents)

should return a const nsModuleComponentInfo*. Pls also rename the out
param to 'outNumComponents'.

Index: nsCocoaBrowserService.h
+class SecurityDialogs;
...
+  static SecurityDialogs* sSecurityDialogs;

I thought we didn't need this any more.

+extern nsModuleComponentInfo* GetAppModuleComponentInfo(int* numComponents);
Fix signature, say where the implementation is, and that embedders might want to
provide their own, in a comment.

+    nsCOMPtr<nsIGenericFactory> componentFactory;
+    NS_NewGenericFactory(getter_AddRefs(componentFactory), &(componentInfo[i]));

Check for failure.

The rest looks good!

Comment 15

[Brian Ryner (not reading)]

Attachment: patch v3.2

Implemented Simon's suggestions; also removed a couple of other #include's that
are no longer needed in nsCocoaBrowserService.h.

Comment 16

[Simon Fraser [no longer active]]

+static nsModuleComponentInfo components[] = {

Still need a 'const' there:
  static const nsModuleComponentInfo components[] = {

maybe also use a name which is less likely to conflict in the static build:

  static const nsModuleComponentInfo ChimeraEmbeddingComponents[] = {

+    rv = NS_NewGenericFactory(getter_AddRefs(componentFactory),
&(componentInfo[i]));
+    if (NS_FAILED(rv))
+      return rv;

Should probably assert on failure, otherwise some poor embedder will waste hours
trying to find the silent failure point.

Fix those, and r=sfraser

Comment 17

[Brian Ryner (not reading)]

> maybe also use a name which is less likely to conflict in the static build:

This can't conflict; the symbol is local to AppComponents.o.

Comment 18

[Brian Ryner (not reading)]

checked in.

Comment 19

[Mike Pinkerton (not reading bugmail)]

v