294530 - nsISerializable.read is not a good COM signature

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P4)

Description

[Benjamin Smedberg]

In my security playgound, I've been playing with nsISerializable objects, and
I've found that the signature nsISerializable.read is not very good. It requires
that you "createInstance" the object first, even when you normally would not be
able to make that object via createInstance. The object's internal state might
be very undefined.

Instead, I think that serializable objects should use an alternate factory that
creates an object from a stream in one motion (and I have already implemented
what I think should happen: it's pretty simple).

This is 1.9 fodder, ignore this bug until 1.8 is gone.

Comment 1

[Brendan Eich [:brendan]]

I can't ignore it. ;-)

The design was intentionally aimed at XPCOM implementations, which generally
*must* support creation with no extra args, followed by Init() or Read().  This
was with jband, at least, in the loop, IIRC.  So I have to question the "[bad]
COM signature" assertion, but I'm not a big COM expert.

/be

Comment 2

[Benjamin Smedberg]

Attachment: nsIDeserializingFactory, rev. 1 [1.9 FODDER, DON'T WASTE YOUR 1.8 TIME]

The "ordinary" COM constructor is the nsIFactory.createInstance method, which
obviously doesn't allow for paramaterful construction. But if your factory were
to implement interfaces other than nsIFactory, they can implement parameterful
constructors.

Separate init methods are good when hidden behind COM constructor methods, so
that you can throw exceptions, but they shouldn't be reflected into COM.

Comment 3

[Darin Fisher]

> It requires that you "createInstance" the object first, even when you normally 
> would not be able to make that object via createInstance. The object's internal 
> state might be very undefined.

OK, I'll bite.  What does this mean exactly?  Why would an object not have a
reasonable initial state?  We have plenty of other components that are useless
until initialized (e.g., nsFileInputStream).  Can you provide more concrete
examples of where the existing API is insufficient or awkward?

Comment 4

[Benjamin Smedberg]

In the current tree, the best example is nsPrincipal. You're never supposed to
createInstance() a nsPrincipal directly. If you try to use nsIPrincipal on an
"uninitialized" nsPrincipal you certainly won't get the expected behavior, and
you may crash. Basically, you should only be able to create a nsIPrincipal with
the methods on nsIScriptSecurityManager or through a specialized deserializer
which hands back a fully-initialized object.

I encountered this in practice on my security branch with "evidence" objects
somewhat similar to nsPrincipal. I have things called capsIDomainEvidence
objects that should be serializable. BUT, each domain has only one "singleton"
capsIDomainEvidence object. If this object is serialized and deserialized with
createinstance there will probably be two or more objects hanging around for the
same domain. The solution is to have the deserializer check for the singleton
and return the existing object if it has already been created, or create a new
one if it hasn't.

Comment 5

[Darin Fisher]

OK, post 1.8 :)

Comment 6

[Brendan Eich [:brendan]]

Ok, I have to say something again, I can't ignore this until 1.9.

First, I handled singletons in the nsISerializable/FastLoad design, I had to:
consider the system principal.  Look for SINGLETON in nsIClassInfo.idl and in
xpcom/io/nsFastLoadFile.cpp.

Second, it's ok with me to have more than one serializable pattern, especially a
factory read-while-constructing one.  Don't get me wrong.  But I am objecting:

Third: what are the XPCOM rulez, please?  If createInstance is valid to call
without any special constructor form, then the new instance should be in a
well-defined state, even if not all that usable.  If some classes do not support
a usable "nullary-created" instance, then we really ought to have a nsIClassInfo
flag distinguishing those classes from others.

Let's play by the rules, know the rules, and amend the rules when we have to
break them.  Having already stared down the singleton issue four years ago (was
it that long?  Almost), it would be good if all the history were known before
new rules were promulgated.

/be

Comment 7

[timeless]

Comment on attachment 183873 [details] [diff] [review]
nsIDeserializingFactory, rev. 1 [1.9 FODDER, DON'T WASTE YOUR 1.8 TIME]

first, i don't think i like this pattern at all. can i implement it in js?

>Index: xpcom/ds/nsSupportsArray.cpp
>@@ -215,55 +216,40 @@ nsSupportsArray::Create(nsISupports *aOu
>+  nsRefPtr<nsSupportsArray> array = new nsSupportsArray();
check for failure before you crash:
>+  rv = array->SizeTo(count);

>Index: content/xul/document/src/nsXULPrototypeDocument.cpp
>@@ -347,67 +344,77 @@ nsXULPrototypeDocument::NewXULPDGlobalOb
>+NS_DeserializeXULPrototypeDocument(nsIObjectInputStream* aStream,
i'm fairly certain these can fail:
>+        proto->mStyleSheetReferences->AppendElement(referenceURI);
>+        proto->mOverlayReferences->AppendElement(referenceURI);

Comment 8

[Benjamin Smedberg]

Sure, this pattern is easy to implement in JS. Your factory object implements
nsIFactory and nsIDeserializingFactory.

Comment 9

[Benjamin Smedberg]

1) My evidence objects are "singleton per domain", so the classinfo singleton
flag doesn't help. Maybe I shouldn't have used the word "singleton" since it
confused the issue.

3) If a class implements createInstance, it *should* be valid to call without
any special constructors. However, createInstance should not be implemented for
my new evidence objects, or for nsPrincipal (that's what this patch implements;
createInstance now fails on the nsPrincipal CID).

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 183873 [details] [diff] [review]
nsIDeserializingFactory, rev. 1 [1.9 FODDER, DON'T WASTE YOUR 1.8 TIME]

Clearing old review request flags. I suspect the design evolution pursued here is no longer wanted. If it is I would again prefer a design doc that covered the existing XPCOM space as well as the changes to it. But since we're changing XPCOM's memory management and C++ bindings, and probably doing other things to avoid XPCOM or reduce its costs, this design conversation can go on in wiki.m.o linked from http://wiki.mozilla.org/Mozilla_2.

/be

Comment 11

[Benjamin Smedberg]

I think I do eventually want the design pattern here, instead of the "arbitrary createInstance" pattern, but if so, I'll come up with a new bug, since this patch is hopelessly bitrotted.