211801 - drop supporting of resource factories

Status: RESOLVED WORKSFORME

(MailNews Core :: Backend, defect, P3)

Description

[Axel Hecht]

The rdf security review showed that resource factories are basically a bad idea.
The resources are globally unique and webcontent shouldn't instantiate an object
just because it uses a resource belonging to some protocol.
The right thing to do bind more data to a resource are delegates. Those can be
security checked aproapriatly, for example.
I found both mailnews folders and addressbook to use resource factories, I'll file
separate bugs for each.

Comment 1

[Alec Flett]

yay! waterson and I designed delegates for just this purpose (to avoid
instantiating objects when you don't need them)

the work is pretty straight forward:
- design a delegate factory for each object
- (the ugly part) find all the QueryInterfaces in the affected code:
  - when going from nsIRDFResource->object, change the QI to a GetDelegate()
  - when going from object->nsIRDFResource you probably need a GetURI() or
similar method on the client object, and then call rdfService->GetResource()

If I recall correctly, the object should not keep a strong reference to its
"outer" resource - I believe the resources own the delegates. This means there
is going to be a bit of excess bloat due to each resource keeping track of its
delegate (Where as before, each resource WAS the equivalent of the delegate)

A good example of delegates in action is in the mail filter code:
http://lxr.mozilla.org/seamonkey/source/mailnews/base/search/src/nsMsgFilterDataSource.cpp#110

In thinking about this, I'm wondering if we really each resource with a
hashtable of delegates, or if we want just a giant hashtable that maps
{resource, delegate-type} -> delegate - that might avoid the bulk of the bloat.

Comment 2

[Chase Tingley]

I believe the current implementation of delegates uses a linked list, rather
than a hash.

Is this sufficient?  I'd tend to think so -- I can't be sure without
instrumenting the code (after converting factories to delegates), but it seems
unlikely that a resource would have a lot of different delegates at the same
time.  (Especially since any new delegate consumers seem likely to use their own
namespace and URIs..)

DelegateEntry in the nsRDFResource looks like a possible candidate for
nsFixedSizeAllocator or some other pool scheme, though.

Comment 3

[chris hofmann]

adding jst to cc...  he might be a better person than rjc to shepard...  haven't
heard from rjc for awhile...  rjc, are you out there?

Comment 4

[Robert John Churchill]

If just wants this bug, dandy by me!  :)

Comment 5

[Benjamin Smedberg]

Have patch, will travel.

Comment 6

[Benjamin Smedberg]

Attachment: Drop resource factories, patch 1

This patch depends on my RDF-globals patch, bug 241758.

I found that the delegate ownership model is unusable as it currently stands,
so I had to fix it.

Comment 7

[Alec Flett]

Comment on attachment 148369 [details] [diff] [review]
Drop resource factories, patch 1

wow, this is fantastic. Nice cleanup of the delegate stuff too.. but is having
GetDelegate() take a UTF8String advantagous? It is really meant to be a short
ASCII tag, not a special value that comes from an external source.

i.e. mail filters use it and I think the tag they use is just "filter"

I wouldn't want to encourage people to start bringing in external data to pass
to GetDelegate() because it now kind of allows non-ASCII tag names.

Comment 8

[Benjamin Smedberg]

Yeah, I thought about that after I had changed it and was in the middle of the
mailnews cleanup. Probably "string" would be simpler than "AUTF8String". Can you
review on the assumption that I make that change?

By the way, this involves moving nsRDFResource.h/cpp from rdf/util/* to
rdf/base/src. I did this in my local tree with symlinks, and will make sure we
do a CVS-copy to preserve blame.

Comment 9

[Alec Flett]

Comment on attachment 148369 [details] [diff] [review]
Drop resource factories, patch 1

great. sr=alecf with that just being "string"

Comment 10

[Johnny Stenback (:jst)]

Using "string" is a bit dangerous and should IMO be avoided whenever possible.
Imagine someone passes in unicode data from JS, we'll happily strip bits from
each character to make it fit in 7-bits for ASCII, and then ship it along, and
while it probably wouldn't, such situations could end up matching valid ASCII
strings internally.

UTF-8 is good for things like this where we know we only care about ASCII and we
don't want to pay the cost of converting UTF-16 into ASCII internally. UTF-8 is
as cheap internally, w/o the possibility of loosing bits in conversions.

Comment 11

[Darin Fisher]

remember also that A*String is now reference counted under-the-hood, so it can
be  advantageous to use A*String parameters.

Comment 12

[Alec Flett]

hmm.. But really is an internal value string value. Think of it like the ASCII
preference names in nsIPrefBranch. The intention is that they really are ASCII
strings - internal tag names like "filter" or "folder" or "abcard" - I mean
those exact strings, not the name of a filter or the name of a folder.

If JS passed in some Unicode string tag via .GetDelegate() that coincidentally
flattened down to the ASCII string "filter", I'm not sure how that would be a
security problem. They would just get the wrong delegate. If they were malicious
and trying to get at the "filter" delegate in a sneaky way, they would have been
able to get the wrong delegate already by just passing in an "filter"

Besides, I'm not sure there is any guarantee that a UTF8 contractID is even
going to work.

Comment 13

[Axel Hecht]

From the mere insides of it, DelegateEntry stores the key as nsCString (which is
fine, as you don't have to delete the buffer manually then).
So why go and throw away the string length of the key in the interface just to 
do strlen right again afterwards?
I don't care whether it'd be AUTF8String or ACString, though, or if we check
isAscii on debug builds or something.
I just think that a char* is not doing anything better when it comes down to
encodings and we need to compute the length again.

Comment 14

[Benjamin Smedberg]

(In reply to comment #13)
> From the mere insides of it, DelegateEntry stores the key as nsCString (which is
> fine, as you don't have to delete the buffer manually then).

If I change to "string", I would just use a char* buffer, and avoid the length
computation altogether (although I suppose strdup() does a length calculation
internall). This saves two words and some calling overhead.

The additional savings come when mailnews clients can call
resource->GetDelegate("ab-directory") instead of using NS_LITERAL_CSTRING. This
becomes even more convoluted if you're an embedder, where you have to use
nsEmbedCString, which is a pain.

I argue to always use AStrings for out-params, but use "string"s for tag-like
data that only goes "in".

Comment 15

[Alec Flett]

furthermore, tag names tend to be quite short (in the 4-10 character length) so
I question whether strlen("foobar") is much cheapter than NS_LITERAL_CSTRING()

(depending on how many delegates we create, it might be worth storing an
internal buffer a la nsCAutoString. I think the actual number of delegates
should be fairly low)

Comment 16

[Darin Fisher]

It sounds like you guys have good reason to prefer |string|.  I really don't
have that much of an opinion... feel free to ignore me ;-)

Comment 17

[Benjamin Smedberg]

Attachment: Forgot the macro-ized inner impl

Put this in rdf/base/public

Comment 18

[[e-mail]]

I really didn't like that inner class code, and after a lengthy discussion on
irc where bsmedberg was able to shoot down all my previous suggestions I
eventually hit upon a slight improvement to his code. So that you know, my
suggestion was to make the existing XPCOM object the inner of the RDF internal
delegate, and forward all of its addref/release calls to the RDF resource, which
when it was eventually destroyed would release its ref to the internal delegate
thus destroying it and the external delegate. It might even be possible to
template the internal delegate; this is not intended to be a complete working
example but it should give you the basic idea:

class nsRDFResource ... nsCOMPtr<nsIInterfaceRequestor> mDelegateWrapper; ...

template<class T> nsDelegateWrapper: public nsIInterfaceRequestor {
  NS_DECL_ISUPPORTS
  NS_DECL_NSIINTERFACEREQUESTOR
  T mDelegate;
}

template<class T> nsDelegateWrapper::GetInterface(REFNSIID aIID, void **aResult)
{
  return mDelegate.QueryInterface(aIID, aResult);
}

class myDelegate ... NS_DECL_ISUPPORTS ... nsIRDFResource *mResource;

NS_IMPL_ADDREF_USING_AGGREGATOR(myDelegate, mResource);
NS_IMPL_RELEASE_USING_AGGREGATOR(myDelegate, mResource);

Comment 19

[Johnny Stenback (:jst)]

(In reply to comment #12)
> hmm.. But really is an internal value string value. Think of it like the ASCII
> preference names in nsIPrefBranch. The intention is that they really are ASCII
> strings - internal tag names like "filter" or "folder" or "abcard" - I mean
> those exact strings, not the name of a filter or the name of a folder.
> 
> If JS passed in some Unicode string tag via .GetDelegate() that coincidentally
> flattened down to the ASCII string "filter", I'm not sure how that would be a
> security problem. They would just get the wrong delegate. If they were 

Yeah, I don't see any security problems in this case either, but handing out the
wrong delegate seems like a bug, no matter how you turn it. We've got the tools
to do the right thing and prevent ever handing out the wrong delegate, virtually
w/o additional cost, so why not use them?

> Besides, I'm not sure there is any guarantee that a UTF8 contractID is even
> going to work.

For the delegates we care about it'll work (since they're just ASCII). For
others, maybe we just need to throw an exception if someone passes in a
non-ASCII string...

Comment 20

[Alec Flett]

answering the question of "why not use the tools we have..?"

Because it simplifies the API, and lowers the overhead of using nsACString.
Instead of having to say GetDelegate(NS_LITERAL_CSTRING("goop"), ...) I can just
say GetDelegate("goop", ...)

Also, as I said above:
> I wouldn't want to encourage people to start bringing in external data to pass
> to GetDelegate() because it now kind of allows non-ASCII tag names.

I don't want people even being TEMPTED to use non-ASCII data here.

Anyhow, I'm going to have a chance to start reviewing this in the next day or
two. Should I wait for a new patch?

Comment 21

[Benjamin Smedberg]

Attachment: Drop resource factories, version 1.1

Use 'string', incorporate Neil's suggestion for nsRDFDelegateInner.h (no need
to maintain the outer refcnt, just call mInner->AddRef/Release as needed).

Comment 22

[Benjamin Smedberg]

alecf: review bug 241758 before this one

Comment 23

[Peter Van der Beken [:peterv]]

Bsmedberg: do you have patches for existing datasources, like the LDAP one? From
a quick look I think REFERENCE_WEAK would be the best fit for what it currently
does, but I'm not sure yet.
I'm also interested to know how you propose we implement a read-write datasource
with an asynchronous caching delegate factory (like for LDAP) if you remove
ReleaseDelegate. Say a resource gets changed through the datasource, we send the
change to the LDAP server and we need to refetch the delegate.

Comment 24

[Peter Van der Beken [:peterv]]

Actually, hmmm, REFERENCE_WEAK won't work :-/.

Comment 25

[Axel Hecht]

I looked at the patch and here is what I have so far:

I lack some more comments about what the delegate ownership stuff is supposed to
be and to accomplish. I bet I could reverse engineer, but nobody should have to.

I find the comment for REFERENCE_WEAK to be confusing, for example. Sentences 
like "if you release it, it's released" need improvement.
"In the CreateDelegate method, return your real object, not nsIWeakReference."
Don't they need to return an nsISupports anyway? Speaking in terms of casts.

I have no clue whatsoever about REFERENCE_IREQUESTOR. Why do you use that 
interface, who's owning who, lifetimes, uniqueness, all that.

I wonder if we should repeat what [key] and [scheme] are in 
nsIRDFDelegateFactory.idl as well. As we duplicate some of the documentation
from nsIRDFResource.idl already.

On the code side, check to have " " after ",", I saw at least one missing in a
NS_STATIC_CAST. And check for each of your 
#ifdef DEBUG_bsmedberg
if you really need those, esp. if they introduce dependencies (which your QI
impl may do, not sure)

Comment 26

[Axel Hecht]

Comment on attachment 148369 [details] [diff] [review]
Drop resource factories, patch 1

We're working on this incrementally now, see bug 286592

Comment 27

[Benjamin Smedberg]

I give up.

Comment 28

[Daniel Veditz [:dveditz]]

Does that mean this isn't a problem after all? Or just that you aren't going to work on it.

Comment 29

[Benjamin Smedberg]

It is a problem, in mailnews apps (not Firefox), and I can't find a way to fix it without boiling the ocean.

Comment 30

[Josh Aas]

I don't see any reason for this bug to remain private. The dependent and blocking bugs discuss much of what is discussed here and it seems like the major issues have been resolved. If there is anything else of value here we're probably better off examining it publicly.

Comment 31

[Johnny Stenback (:jst)]

Moving this bug to Thunderbird given that this is only a problem in mailnews apps per comment 29.

Comment 32

[Magnus Melin [:mkmelin]]

We removed RDF a long time ago.