109777 - Make sure certificate downloading works immediately

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Kai Engert (:KaiE:)]

The constructor of nsNSSComponent registers content listeners for 4 mime types
in order to support downloading and importing certificates from websites.

If a user tries to download a certificate (for example in preparation to accept
a new Certification Authority) before PSM has initialized, the mime type will be
yet unknown and the user will be prompted to save the certificate to disk (which
is not what is wanted).

If I understand this correctly, to make downloading certs work even if PSM has
not yet been loaded, the downloading of certificates has to be re-implemented
using nsIContentHandler and registered with
NS_CONTENT_HANDLER_CONTRACTID_PREFIX"mimetype" contract ids (See also bug
108600).

Comment 1

[Kai Engert (:KaiE:)]

P1, target 2.2

Comment 2

[Kai Engert (:KaiE:)]

This needs to block 75947.

Comment 3

[Kai Engert (:KaiE:)]

After reading more Mozilla code, I think the current certificate downloading
code is ok, but needs to be extended.

Changing topic from "Re-Implement certificate downloading code" to "Make sure
certificate downloading works immediately".

Comment 4

[Kai Engert (:KaiE:)]

This bug is a blocker for landing PSM startup time reduction.

CC'ing mscott and sspitzer, as I think a change to URILoader should be made.

Comment 5

[Kai Engert (:KaiE:)]

Motivation: Loading of security components must be delayed until actually needed.

Requirement: Downloading of security content types must work correctly at any
time, even if components are not yet loaded.

Problem: Security registers a "content listener", but not before security
components have been loaded. If security content types are being downloaded
before security components are loaded by Mozilla, content types will be yet unknown.

Idea: Define a content handler with a NS_CONTENT_HANDLER_CONTRACTID_PREFIX, that
triggers instantiation of a security object, thus loading the security component.

Next problem: "Content listeners" are searched first. "Content Handlers" are the
fallback option. That means, while the defined handler loads the security
component, the content listener will not work the first time a security content
type is downloaded, but only at later times.

Next idea: Define a new return code, that has the meaning "retry once". In the
security content handler, return the new retry code. Extend URILoader. If
URILoader encounters the retry code, it will retry searching for registered
content handlers once again (but at most once per function call). As meanwhile
the security component has been loaded and registered, the correct content
listeners will be found and everything works.

The attached patch implements that idea.

Alternative: Maybe you will tell me that you don't like my patch, and the
content handler itself should directly handle the content. Actually that was my
initial idea to fix this bug. If you want me to use that alternative strategy,
please help me understand how I can access the content from the handler, I
couldn't find approriate interface methods to access it. And there seems to be
no code in Mozilla implementing a content handler that way.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: nsIContentHandler / URILoader patch for discussion

This patch ignores whitespace for better readability.

Optionally we can replace NS_ERROR_NOT_AVAILABLE with a new yet-to-define error
code like NS_ERROR_RETRY_ONCE.

mscott, sspitzer: do you agree to that change to your code?

Comment 7

[Kai Engert (:KaiE:)]

Another option to fix this bug: We could make sure, immediately during startup
of Mozilla some code from the security component is loaded and excuted and
registers the content listeners for the supported mime types.

But I'm not sure how we should trigger it.

And it would require some security code to be loaded during startup, which is
contrary to what is being requested in bug 75947 (delay loading of PSM to reduce
startup time).

Comment 8

[Scott MacGregor]

Hey Kai,we have a construct called a nsIURIContentHandler. In other parts of the
code we want the ability to handle content when the module for that content type
has not been loaded yet. This sounds exactly like the scenario you are in. We
use this for instance when you click on a mailto link and we haven't loaded the
mailnews module yet. The uriloader sees that a component is registered to
implement nsIURIContentHandler for a specified content type by looking in the
registry (no need to load the module). Then it will load the module and
instantiate your instance of nsIURIContentHandler. 

That's how I'd recommend approaching this problem. That is, implment a
nsIURIContentHandler in the security module for the content type in question.
Look at the way we implement it in mailnews/compose for mailto urls. 

However, content handlers don't allow you to really work with the inital data
stream coming in. They weren't designed to do that. However we do give you the
original window the url is being run from and the load request for the url. Your
implementation of nsIURIContentHandler can abort the current load, then get the
url from the request and reload it in the original window. 

If that starts to look too complicated then we can start looking at some changes
to this interface.

Comment 9

[Kai Engert (:KaiE:)]

Scott, I already understood what you describe. Please let me try again to explain.


> Hey Kai,we have a construct called a nsIURIContentHandler. In other parts of the
> code we want the ability to handle content when the module for that content type
> has not been loaded yet. This sounds exactly like the scenario you are in. We
> use this for instance when you click on a mailto link and we haven't loaded the
> mailnews module yet. The uriloader sees that a component is registered to
> implement nsIURIContentHandler for a specified content type by looking in the
> registry (no need to load the module). Then it will load the module and
> instantiate your instance of nsIURIContentHandler. 


- The Mozilla source does not contain the identifier nsIURIContentHandler.

- I assume you mean nsIContentHandler, which is used by URILoader.

- I already understood that implementing this interface for certain contract IDs
triggers loading of the the security module.


> That's how I'd recommend approaching this problem. That is, implment a
> nsIURIContentHandler in the security module for the content type in question.
> Look at the way we implement it in mailnews/compose for mailto urls. 


Please look at the attached patch.

- I do implement the nsIContentHandler interface, i.e. the HandleContent method.

- It was my intention to use the mechanism you describe to trigger loading of
the module.

- I'm using the approach explained in my comment #5 to define the contract ID
using NS_CONTENT_HANDLER_CONTRACTID_PREFIX, which indeed triggers loading of the
module. (I didn't include that in the patch for some reason).


> However, content handlers don't allow you to really work with the inital data
> stream coming in. They weren't designed to do that. However we do give you the
> original window the url is being run from and the load request for the url. Your
> implementation of nsIURIContentHandler can abort the current load, then get the
> url from the request and reload it in the original window. 
> 
> If that starts to look too complicated then we can start looking at some changes
> to this interface.


- I saw that it is difficult to access the data stream from within that method.
You confirm that.

- That was the reason why I suggested the approach in the patch.

You suggest that we cancel loading and trigger loading again.

My suggestion, as defined in the attached patch, is similar.

Instead of loading again, I suggested to make URILoader look for registered
content listeners once again, when nsIContentHandler::HandleContent returns some
special return code.

This will work, because: When our nsIContentHandler::HandleContent method is
reached, the security component has already been loaded. We use a special
mechanism in our module factory:

Regardless of what interface from our module is requested, our global module
init code will always be executed. This includes registering our content listeners!

Therefore, at the time we return from HandleContent, our content listeners will
be registered. The next time URILoader tries to find the content listeners, it
will find them.

My suggestion is to return a special return code, signaling "try again" to the
URI Loader, causing it to try again to find registered content listeners. I
implemented that in the patch.

Therefore, if you agreed to the suggested change in the patch, we wouldn't need
to do anything special inside HandleContent. Returning the special return code
back to URI Loader, indicating "try again", would be enough.

This approach avoids aborting the load and loading it again, which even results
in faster browser behaviour for the first security mime type downloaded.

I already tested my patch. It works. The question is, are you willing to accept
it? I'm asking, because you are the owner of the URILoader module.

If you don't accept my change, I will need to implement the HandleContent in the
way you describe it, by aborting and loading again. I think my approach is
clean, but I will respect your wish to not change URILoader if you think we
should not add my patch.

Alternatively, there might even be a third possibility. Bradley Baetz suggested
to look at nsIDocumentLoaderFactory, which I haven't done yet.

Comment 10

[Kai Engert (:KaiE:)]

One more detail: The "special mechanism in our module factory" is not yet
checked in. It is part of the patch in bug 75947, which this bug blocks.

Comment 11

[Kai Engert (:KaiE:)]

Scott: I object against aborting the load and trying to reload, as you suggested it.

The mime type we are downloading can be the result of a post operation.

And Verisign even uses multipart pages, where one element is an certificate,
which would us require to reload the whole multipart document.

We must not post form data twice, and therefore our first load must succeed.

If a content handler can't handle the content, then the content listener must
handle it.

If content listeners can't be registering using a contract ID, we should either
invent that, or, much more simple, just take my patch, which makes the URILoader
retry to find a content listener, after the content handler had the chance to
register everything that is required.

Please give me some feedback, whether my changes are acceptable.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Suggested fix

Scott, I think you will like this patch.

Instead of adding a "retry hack" to your code, I had a different idea:
  Allow content listeners to be loaded using contract IDs.

In nsURILoader::DispatchContent, after the code has iterated over all already
registered content listeners, another try is made to instantiate an
nsIURIContentListener over a new contract ID.

The attached patch works perfectly together with my patch from bug 75947. I
added some debugging output to the library loader. I can see that the security
component is not loaded. When I try to download one of the registered mime
types, I see the security library gets loaded, and the download works
immediately.

Scott, what do you think? Can you please review the URI loader changes?

Comment 13

[Scott MacGregor]

Comment on attachment 58958 [details] [diff] [review]
Suggested fix

yes I do like this patch a lot more than the others. They only thing I think we
might want to change is to use the category manager for getting the content
listener instead of using do_CreateInstance directly. That will allow the
ability for multiple consumers to register themselves on the same content type.

Comment 14

[Scott MacGregor]

Attachment: uriloader change using the category manager

I just took Kai's patch to the uriloader and made it use the category manager.
You'll want to modify your registration code in PSM to register your PROGID
with the category manager. Something like the following in your psm factory:

static NS_METHOD RegisterContentListeners(nsIComponentManager *aCompMgr,
nsIFile *aPath, const char *registryLocation, 
				       const char *componentType, const
nsModuleComponentInfo *info)
{
  nsresult rv;
  nsCOMPtr<nsICategoryManager> catman =
do_GetService(NS_CATEGORYMANAGER_CONTRACTID, &rv);
  if (NS_FAILED(rv)) return rv;
  nsXPIDLCString previous;
 
  return catman->AddCategoryEntry("external-uricontentlisteners", a content
type, info->mContractID, PR_TRUE, PR_TRUE, getter_Copies(previous));
}

then when you register the module you'll pass in RegisterContentListeners as an
argument in your components array:

static nsModuleComponentInfo components[] =
{
  { "psm content listeners",
    psm_listener__CID,
    the contract id,
    optional constructor,
    RegisterContentListeners
  },
};

Comment 15

[Scott MacGregor]

Attachment: same patch as before, just changed the category's name

Comment 16

[Scott MacGregor]

With this latest patch you should use:

NS_CONTENT_LISTENER_CATEGORYMANAGER_ENTRY

as the name of the category entry in your factory code when you register
yourself with the category manager. 

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Consolidated fix

Scott, thanks a lot, this helped me.
I am attaching a new consolidated working patch.

Scott, as you created the URILoader changes, who should review them?

Javi, can you please review the security changes?

Comment 18

[Scott MacGregor]

Comment on attachment 59514 [details] [diff] [review]
Consolidated fix

sr=mscott

For kicks can you try clicking on a mailto url and making sure the compose
window still comes up? Just want to make sure we didn't break uri content
handlers with this change to the uriloader

Comment 19

[Scott MacGregor]

From a performance point of view, this change to the uriloader shouldn't hurt us
because the new code is ONLY executed if all the currently open windows have
said they don't want to handle the content and we're about to hit the component
manager to see if a content handler is registered for the content type. It is
rare that we get this far into the uriloader since most urls load in either the
existing window or get re-targeted to a window that is already up. 

Comment 20

[Javier Delgadillo]

Comment on attachment 59514 [details] [diff] [review]
Consolidated fix

r=javi

Comment 21

[Kai Engert (:KaiE:)]

Scott, mailto URLs still work.

Comment 22

[Kai Engert (:KaiE:)]

checked in, fixed

Comment 23

[John Unruh]

Verified.