74945 - context not being passed into calls to the loadgroup

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Bradley Baetz]

While fixing bug 74666, I noticed that the call to mLoadGroup->RemoveRequest was
passing nsnull (instead of mUserContext).

An lxr regexp search for mLoadGroup.*ctxt and mLoadGroup.*context shows that
there are two places using the former and 8 using the latter which get this
correct. There are 31 occurrences of mLoadGroup->{Add,Remove}, which gives us
about a 33% success rate.

The only places which seem to pass through something non-null to AsyncOpen are:

/extensions/psm-glue/src/nsPSMComponent.cpp, line 887
/xpfe/components/directory/nsDirectoryViewer.cpp, line 1432
some testing stuff, which is how I found bug 74666
/xpfe/components/search/src/nsInternetSearchService.cpp

It appears that the testing code uses this construct more often than the actual
browser code does.

All the above are LXR searches and so miss calls spread over multiple lines.

So, I think that clarifying/defining the use of the context paramater would be
useful. :)

It is also quite possible that lots of other pass the wrong context through (ie
forwarding the context through instead of passing in the context given to
AsyncOpen) - I've found several of these. The fact that only TestProtocols seems
to actually show this up probably means that either:

1. Lots of code could benefit from using a context, instead of keeping some sort
of lookup table locally.
2. Most code only runs one request at a time, and so there is no benefit to
using the context paramater as opposed to keeping it locally.

I don't have any evidence for 1, although I started using it in the directory
viewer (which loads multiple directories at once), and managed to remove a
couple of elements from the per-lookup class, after simply getting the URI from
the nsIRequest was no longer possible/enough to give me all the information I
wanted.
2. sounds more likely (the main uses of places which did use the context were
places like LDAP), but again, I have no evidence to suggest this.

Regardless, the loadgroup stuff should be fixed.

Comment 1

[Darin Fisher]

bbaetz: can you connect any real bugs to this, or is this merely a matter of
making sure we're implementing our interfaces?  having real bugs resulting
from this problem would definitely bump up this bug's priority ;-)

Comment 2

[Bradley Baetz]

I can't find any (except for the stuff in netwerk/test) That is probably due to
the fact that almost noone else uses the context.

Using the context isn't essential - all the test programs could be written to do
without it, and just have an extra class member each.

So fixing this probably isn't a high priority, but documenting what it is meant
to do would be a good step.

Comment 3

[Darin Fisher]

agreed.. thanks for investigating this.

Comment 4

[Bradley Baetz]

.. in fact, I'm sort of puzzled by the whole loadgroup arragement. There are 6
callers of ns_newloadgroup, and only the docloader passes an observer down into
it (via Init).

The context paramater which I'm worrying about here is passed back into the
observer (and the docloader just ignores this paramater). This sounds wrong -
AFAICS the context never gets sent back to the caller (and there is no way of
doing so). There is also no way for the load group to be given a context by the
observer. So I can't really see the point for this, at all, and everyone passing
nsnull through is as good a suggestion as any.

Does this API need to be rethought?

Comment 5

[benc]

mass move, v2.
qa to me.

Comment 6

[Darin Fisher]

-> future... will worry about this later.

Comment 7

[Darin Fisher]

-> default owner

Comment 8

[Patrick McManus [:mcmanus]]

asyncopen2() is removing that argument entirely