361087 - remove the nsIXULPrototypeDocument interface

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Nickolay_Ponomarev]

nsIXULPrototypeDocument isn't really used outside mozilla/content/xul (except for nsChromeProtocolHandler, where nsIXULPrototypeCache::GetPrototype is used to check if the document being loaded is in the XUL cache):

http://mxr-test.landfill.bugzilla.org/seamonkey/search?string=nsIXULPrototypeDocument

Can we use the actual class (nsXULPrototypeDocument) in the content/xul code instead and add another method to nsIXULPrototypeCache that can be used by the chrome protocol code instead of nsIXULPrototypeCache::GetPrototype?

bz told me to consult with Neil and Neil, so please comment.

Comment 1

[Boris Zbarsky [:bzbarsky]]

ccing bsmedberg too, if the chrome protocol code is involved.  ;)

Comment 2

[Benjamin Smedberg]

It would be good, from my POV, to avoid ever asking the chrome registry to do channel loads if we're already in the prototype cache. That was we can remove the hacks that do null channel loads from the chrome protocol handler.

Comment 3

[Nickolay_Ponomarev]

bsmedberg, I didn't understand what you said. Do you mean the chrome protocol handler shouldn't return a fake channel for cached document? If so, any suggestions no how it should be done?

In the meantime, I've changed the code in chrome/ to use the new method, nsIXULPrototypeCache::IsCached.

Comment 4

[Nickolay_Ponomarev]

Attachment: remove the interface as suggested + decomtaminate the methods that were on the interface

Anyone wants to review this?

I didn't touch the Set/GetHeaderData methods, are they needed at all? What breaks because they are not implemented? Do we have a bug on it?


Also, I have a XXX in nsXULPDGlobalObject::SetGlobalObjectOwner in the patch. nsXULPDGlobalObject gets its principal from nsXULPrototypeDocument, but the document is set via nsIScriptGlobalObject::SetGlobalObjectOwner, which is not guaranteed to get passed the nsXULPrototypeDocument, so the static case I have in place is wrong.

I'd rather not have a separate interface for this, unless absolutely needed, since it seems that only nsXULPrototypeDocument can in fact be the 'global object owner' (and the old code relied on that, if I'm reading it correctly).

If I'm right, maybe put NS_NOTREACHED() in nsXULPDGlobalObject::SetGlobalObjectOwner and make the proto document initialize nsXULPDGlobalObject through other means?

Comment 5

[Benjamin Smedberg]

I wish that we wouldn't ask the chrome protocol handler for a channel at all if we have a prototypecached document. It causes all sorts of problems when we start doing view-source or XSLT or other important things with chrome channels for XUL documents.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Who is "we"?

The code opening the channel has no way to know that XUL is involved, really.

Comment 7

[Benjamin Smedberg]

Neither does the chrome protocol handler... it just checks the prototype cache for a prototype. I wish that check were performed somewhere higher up.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If we did it higher up, we couldn't have about: and company redirecting to chrome:....

Comment 9

[Nickolay_Ponomarev]

Attachment: updated, builds on linux

Comment 10

[Nickolay_Ponomarev]

Comment on attachment 246438 [details] [diff] [review]
updated, builds on linux

of course I couldn't generate the patch properly and the interface has changed anyway in bug 354693.

Comment 11

[Nickolay_Ponomarev]

Attachment: patch v1.2

Updated, finally. I would really appreciate an answer to the question in comment 4.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Which one?  There are four separate questions there.

Comment 13

[Nickolay_Ponomarev]

I'm sorry. The one about nsXULPDGlobalObject::SetGlobalObjectOwner, am I correct thinking it can only be called with an nsXULPrototypeDocument and does my proposed solution with NS_NOTREACHED and a separate method for initializing the principal on nsXULPDGlobalObject sound fine?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> am I correct thinking it can only be called with an nsXULPrototypeDocument

I believe so.  And yes, a separate method should be ok.

Comment 15

[Nickolay_Ponomarev]

Attachment: patch

Thanks Boris.

Comment 16

[Nickolay_Ponomarev]

Comment on attachment 247105 [details] [diff] [review]
patch

Forgot to ask for review.

Comment 17

[Nickolay_Ponomarev]

Attachment: patch

updated to the tip.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 248983 [details] [diff] [review]
patch

Very nice patch, good to see interfaces disappear.

>Index: content/xul/document/src/nsXULDocument.cpp
> nsXULDocument::OnDocumentParserError()
...
>+    nsCOMPtr<nsIURI> uri = mCurrentPrototype->GetURI();
>+    PRBool isChrome = IsChromeURI(uri);
>+    if (isChrome) {

Just do |if (IsChromeURI...|

>+nsXULDocument::GetScriptGlobalObjectOwner(nsIScriptGlobalObjectOwner** aGlobalOwner)
...
>+    *aGlobalOwner = mMasterPrototype;
>+    NS_IF_ADDREF(*aGlobalOwner);
>     return NS_OK;

Nit: You can do |IS_IF_ADDREF(*aGlobalOwner = mMasterPrototype);|

>@@ -1914,18 +1893,22 @@ nsXULDocument::Init()
> 
>         gRDFService->GetResource(NS_LITERAL_CSTRING(NC_NAMESPACE_URI "persist"),
>                                  &kNC_persist);
>         gRDFService->GetResource(NS_LITERAL_CSTRING(NC_NAMESPACE_URI "attribute"),
>                                  &kNC_attribute);
>         gRDFService->GetResource(NS_LITERAL_CSTRING(NC_NAMESPACE_URI "value"),
>                                  &kNC_value);
> 
>-        rv = CallGetService(kXULPrototypeCacheCID, &gXULCache);
>+        nsIXULPrototypeCache* cache;
>+
>+        rv = CallGetService(kXULPrototypeCacheCID, &cache);
>         if (NS_FAILED(rv)) return rv;
>+
>+        gXULCache = NS_STATIC_CAST(nsXULPrototypeCache*, cache);

This is not really good. You shouldn't instantiate something through XPCOM and then assume that it's of a certain implementation. 

Can't you keep gXULCache as an nsIXULPrototypeCache?

The alternative is to remove the XULPrototypeCache CID entirely so that it can't be instantiated or overridden through XPCOM. But you should get signoff from bsmedberg and bz for that.

> nsXULDocument::AddPrototypeSheets()
> {
>     nsresult rv;
> 
>-    nsCOMPtr<nsISupportsArray> sheets;
>-    rv = mCurrentPrototype->GetStyleSheetReferences(getter_AddRefs(sheets));
>-    if (NS_FAILED(rv)) return rv;
>-
>-    PRUint32 count;
>-    sheets->Count(&count);
>-    for (PRUint32 i = 0; i < count; ++i) {
>-        nsISupports* isupports = sheets->ElementAt(i);
>-        nsCOMPtr<nsIURI> uri = do_QueryInterface(isupports);
>-        NS_IF_RELEASE(isupports);
>+    nsCOMArray<nsIURI> sheets = mCurrentPrototype->GetStyleSheetReferences();

This is very wasteful. Make it

nsCOMArray<nsIURI>& sheets = ...

to avoid copying the entire array. Though to stay on the safe side, you should probably make GetStyleSheetReferences return a |const nsCOMArray<nsIURI>&| so that no-one accidentally mucks around with the array.

>Index: content/xul/document/src/nsXULPrototypeCache.h
>+class nsXULPrototypeCache : public nsIXULPrototypeCache,
>+                                   nsIObserver
>+{

If you do keep this (see above comment) then I suspect some of the methods in this class can be made non-virtual. Though feel free to leave that to a later patch if you want. But do add a comment to that extent if so.

>Index: content/xul/document/src/nsXULPrototypeDocument.cpp
> nsXULPrototypeDocument::Write(nsIObjectOutputStream* aStream)
...
>+    for (PRUint32 i = 0; i < count; ++i) {
>+        rv |= aStream->WriteCompoundObject(mStyleSheetReferences[i],
>+                                           NS_GET_IID(nsIURI), PR_TRUE);
>     }

Don't declare variables inside the |for| conditions. Different compilers still deal with that differently so you're likely to get warnings or compile errors on some platforms. Always declare variables before the loop, especially if they are used after the loop.

Same thing twice further down in the same function.

Looks good otherwise, but I'd like to see a new patch with this fixed, especially the gXULCache thing.

Comment 19

[Nickolay_Ponomarev]

Thanks for the review!

(In reply to comment #18)
> >@@ -1914,18 +1893,22 @@ nsXULDocument::Init()
...
> >-        rv = CallGetService(kXULPrototypeCacheCID, &gXULCache);
> >+        nsIXULPrototypeCache* cache;
> >+
> >+        rv = CallGetService(kXULPrototypeCacheCID, &cache);
> >         if (NS_FAILED(rv)) return rv;
> >+
> >+        gXULCache = NS_STATIC_CAST(nsXULPrototypeCache*, cache);
> 
> This is not really good. You shouldn't instantiate something through XPCOM and
> then assume that it's of a certain implementation. 
> 
I thought it was ok when instantiating by CID (as opposed to contract ID). Is it incorrect?

> Can't you keep gXULCache as an nsIXULPrototypeCache?
> 
Yes I can, but I thought it was a good idea to remove the methods not used outside of content/xul from the nsIXULPrototypeCache and just use the concrete class there (while still using the interface from chrome protocol handler).

I guess I'll change it back to nsIXULPrototypeCache for now and leave this to a separate bug.

> The alternative is to remove the XULPrototypeCache CID entirely so that it
> can't be instantiated or overridden through XPCOM. But you should get signoff
> from bsmedberg and bz for that.
> 
I won't be able to get it from the chrome protocol handler then, will I?

> >+    nsCOMArray<nsIURI> sheets = mCurrentPrototype->GetStyleSheetReferences();
> 
> This is very wasteful. Make it
> 
Oops.

> >Index: content/xul/document/src/nsXULPrototypeCache.h
> >+class nsXULPrototypeCache : public nsIXULPrototypeCache,
> >+                                   nsIObserver
> >+{
> 
> If you do keep this (see above comment) then I suspect some of the methods in
> this class can be made non-virtual. Though feel free to leave that to a later
> patch if you want. But do add a comment to that extent if so.
> 
Yes, I didn't want to touch this interface in this patch, especially since I had to move the nsXULPrototypeCache declaration from the .cpp to .h file, so any changes I make to this class are hard to track.

> >Index: content/xul/document/src/nsXULPrototypeDocument.cpp
> > nsXULPrototypeDocument::Write(nsIObjectOutputStream* aStream)
> ...
> >+    for (PRUint32 i = 0; i < count; ++i) {
> >+        rv |= aStream->WriteCompoundObject(mStyleSheetReferences[i],
> >+                                           NS_GET_IID(nsIURI), PR_TRUE);
> >     }
> 
> Don't declare variables inside the |for| conditions. Different compilers still
> deal with that differently so you're likely to get warnings or compile errors
> on some platforms. Always declare variables before the loop, especially if they
> are used after the loop.
> 
Heh, so much for all the talk about modern compilers...

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, you're right on the CID thing, feel free to leave things as is. Please do attach a new patch with the remaining stuff fixed though.

Comment 21

[Nickolay_Ponomarev]

Attachment: patch

review comments fixed + bump IIDs (I remembered to do it this time!)

Comment 22

[Nickolay_Ponomarev]

Filed bug 364329 for nsIXULPrototypeCache decomtamination.

Comment 23

[Nickolay_Ponomarev]

Attachment: patch

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 249094 [details] [diff] [review]
patch

Looks good, thanks for the patch. r/sr=sicking

Comment 25

[:Gijs (he/him)]

Last patch checked in. Condensed CVS log follows.

Checking in mozilla/chrome/src/nsChromeProtocolHandler.cpp;
new revision: 1.122; previous revision: 1.121
Checking in mozilla/content/xul/content/src/nsXULElement.cpp;
new revision: 1.675; previous revision: 1.674
Checking in mozilla/content/xul/content/src/nsXULElement.h;
new revision: 1.232; previous revision: 1.231
Checking in mozilla/content/xul/document/public/Makefile.in;
new revision: 1.12; previous revision: 1.11
Checking in mozilla/content/xul/document/public/nsIXULContentSink.h;
new revision: 1.10; previous revision: 1.9
Checking in mozilla/content/xul/document/public/nsIXULDocument.h;
new revision: 1.38; previous revision: 1.37
Checking in mozilla/content/xul/document/public/nsIXULPrototypeCache.h;
new revision: 1.46; previous revision: 1.45
Removing mozilla/content/xul/document/public/nsIXULPrototypeDocument.h;
new revision: delete; previous revision: 1.19
Checking in mozilla/content/xul/document/src/nsXULContentSink.cpp;
new revision: 1.174; previous revision: 1.173
Checking in mozilla/content/xul/document/src/nsXULDocument.cpp;
new revision: 1.743; previous revision: 1.742
Checking in mozilla/content/xul/document/src/nsXULDocument.h;
new revision: 1.188; previous revision: 1.187
Checking in mozilla/content/xul/document/src/nsXULPrototypeCache.cpp;
new revision: 1.61; previous revision: 1.60
Checking in mozilla/content/xul/document/src/nsXULPrototypeCache.h;
initial revision: 1.1
Checking in mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp;
new revision: 1.85; previous revision: 1.84
Checking in mozilla/content/xul/document/src/nsXULPrototypeDocument.h;
initial revision: 1.1
Checking in mozilla/layout/build/nsLayoutModule.cpp;
new revision: 1.166; previous revision: 1.165
Checking in mozilla/rdf/chrome/src/nsChromeProtocolHandler.cpp;
new revision: 1.114; previous revision: 1.113

Comment 26

[Nickolay_Ponomarev]

Fixed. Thanks everyone for the help.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Is it possible that this caused a new topcrasher [@ nsXULDocument::ResumeWalk] (2006122304 build -> )

Comment 28

[Nickolay_Ponomarev]

(In reply to comment #27)
> Is it possible that this caused a new topcrasher [@ nsXULDocument::ResumeWalk]
> (2006122304 build -> )
> 
Possible, but I don't see how yet... Filed bug 364900 for it.

Comment 29

[Nickolay_Ponomarev]

(In reply to comment #28)
> (In reply to comment #27)
> > Is it possible that this caused a new topcrasher [@ nsXULDocument::ResumeWalk]
> > (2006122304 build -> )
> > 
> Possible, but I don't see how yet... Filed bug 364900 for it.
> 
Posted a patch over there, it was indeed a fallout from this patch.