272471 - [FIX]Revamp the docshell/docloader relationship

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

The plan is as follows:

1)  Make docshell inherit from docloader
2)  Remove some methods on scriptable apis that should never have been exposed
3)  Simplify a bunch of code
4)  Take the first step to finally killing off nsIWebShell

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167472 [details] [diff] [review]
Proposed patch

jst, darin, could you review?  biesi, any comments you have would also be quite
welcome!

Summary of changes:

nsIDocumentLoader/nsDocLoader:
  * remove createDocumentLoader and all callers
  * remove destroy() method
  * remove ability to set container (leave the getter for now,
    but I think we can remove that too).
  * Add some utility methods
  * Convert child storage to not hold refs to the kids
  * Make QI threadsafe (As docshell's was)

nsIURILoader/nsURILoader:
  * remove getDocumentLoaderForContext
  * remove SetupLoadCookie
  * remove docloader contractid (so they can't be instantiated on their
    own, except via the service's contractid).

nsIDocShell*/nsDocShell:
  * Inherit from nsDocLoaderImpl
  * Clean up inheritance tree and QI/GetInterface impls to eliminate
    things that nsDocLoaderImpl inherits from or QIs to or hands out
    via GetInterface.
  * Fix GetInterface and QI to forward to nsDocLoaderImpl
  * Make newly-created docshells be kids of the root docloader
  * Remove ability to set parent treeitems (forces use of
    addChild/removeChild apis).
  * Eliminate mLoadCookie and fix relevant code to just use |this|.
  * Enforce that all kids of docshells are nsDocLoaderImpl instances


nsIWebShell/nsWebShell:
  * Remove unused code, clean up GetInterface impl a lot.
  * Remove GetDocumentLoader

Mailnews:
  * Stop using GetDocumentLoader and nsIWebShell in some places

nsWebBrowser:
  * Make changes to deal with nsIDocShellTreeItem api change.  Plan is
    to move the parent of treeitem altogether, since it makes no sense
    for nsWebBrowser.

Comment 3

[Darin Fisher]

Very interesting.  Expect that it'll take me some time to review this carefully.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Makes sense.  This definitely needs a careful review...  biesi said he'd look
too.  My only constraint is that I will likely be unable to check anything in
between Dec 10 and Jan 2, but it seems that 1.8a6 closes on Jan 4, so I can
still get it in before freeze if it gets reviews by then.  I'd dearly like to
get this (and better yet the final removal of nsIWebShell/nsIWebShellContainer
that I can do once this lands) in 1.8a6...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167472 [details] [diff] [review]
Proposed patch

I noticed a place where I forgot a null-check.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: With that null-check

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167874 [details] [diff] [review]
With that null-check

Wrong file....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Right file this time

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 167878 [details] [diff] [review]
Right file this time

uriloader/base/nsIDocumentLoader.idl
   void fireOnLocationChange(in nsIWebProgress aWebProgress,

this can now be removed from the idl and be a protected method it seems

there won't be much left here :-)

uriloader/base/nsDocLoader.h
+    // Needed to deal with ambiguous inheritance from nsISupports...
+    static nsISupports* GetAsSupports(nsDocLoaderImpl* aDocLoaderImpl) {

I find this kinda ugly... but ok. but, can't you make this an instance method?
then you need no parameter..

+    nsIDocumentLoader* ChildAt(PRInt32 i) {
+	 return NS_STATIC_CAST(nsDocLoaderImpl*, mChildList[i]);

wish we had a template array class for stuff like this... seems like it should
be pretty easy to wrap one around nsVoidArray...


Hm... looks like you are eliminating SetLoadCookie. this slightly worries me...
code might have made assumptions about it... it does exist on a frozen
interface, after all... there's also:
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsURLFetcher.cpp#1
93

and there are various other impls that you could clean up, and eliminate member
vars from...

that said, I like the change :-)

uriloader/base/nsCURILoader.idl
hm... am I misreading this, or does this file contain _two_ #defines for
NS_DOCUMENTLOADER_SERVICE_CID, only one of which you are removing?

this file could use more comments... well, not in this bug :)

uriloader/base/nsDocLoader.cpp
+  // XXXbz now that NS_IMPL_RELEASE stabilizes by setting refocount to 1, is

"refcount" :-)

+  if (!aSupports) {
+    return nsnull;

is that nullcheck needed?

+  nsDocLoaderImpl* ptr;
+  aSupports->QueryInterface(kThisImplCID, (void**) &ptr);

hmm, with use of NS_DEFINE_STATIC_IID_ACCESSOR you could use CallQueryInterface
here...
And that would also simplify AddDocLoaderAsChildToRoot (hm.. maybe...)

 nsDocLoaderImpl::GetContainer(nsISupports** aResult)
loader.container == loader? nice :)


+    for (i=0; i < count; i++)

could declare the var in the for, and please add a space around =

also, can you move the count decl to where it's used?

 NS_IMETHODIMP
 nsDocLoaderImpl::GetDOMWindow(nsIDOMWindow **aResult)
+  return CallGetInterface(this, aResult);

I don't like this... why not return null and override this in the docshell?
(or return error here)

docshell/base/nsDocShell.h
+	 // Need this here because otherwise nsIWebNavigation::Stop
+	 // overrides the docloader's Stop()


more to come!

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 167878 [details] [diff] [review]
Right file this time

docshell/base/nsDocShell.h
+    NS_FORWARD_NSISECURITYEVENTSINK(nsDocLoaderImpl::);

that trailing comma will cause problems with gcc 3.4 and -pedantic

hm, docshell implements ridiculously many interfaces...

docshell/base/nsWebShell.h
+    // That overrides an nsIDocumentLoader method, so redeclare and forward
+    NS_IMETHOD GetContainer(nsISupports** aContainer) {
+	 return nsDocLoaderImpl::GetContainer(aContainer);

I don't understand this comment... can you explain?
what's "that"?

docshell/base/nsDocShell.cpp
    else {
-	 return QueryInterface(aIID, aSink);
+	 return nsDocLoaderImpl::GetInterface(aIID, aSink);

hm, interesting... I assume you verified that this is safe :-)

+    // Only allow setting the type on root docshells

I see that this code did that before too, but why?

+    nsCOMPtr<nsIDocShellTreeItem> parent =
+	 do_QueryInterface(GetAsSupports(mParent));

hmm, you're inconsistent with constructor-style vs using = here...

+	 nsCOMPtr<nsIWebProgress> webProgress =
+	     do_QueryInterface(GetAsSupports(this));

same...

+    nsRefPtr<nsDocLoaderImpl> childAsDocloader = GetAsDocLoaderImpl(aChild);

can you use consistent capitalization here? ;) (Docloader vs DocLoader)

+    // Make sure to remove the child from its current parent.
+    nsDocLoaderImpl* childsParent = childAsDocloader->GetParent();
+    childsParent->RemoveChildGroup(childAsDocloader);

what if the child does not have a parent?

oh... guess all docloaders have a parent (the root, if nothing else); maybe you
should mention that somewhere (GetParent documentation, or here). does that
mean GetParent() should be renamed to Parent()?

+    nsresult res = AddChildGroup(childAsDocloader);
+    NS_ENSURE_SUCCESS(res, res);

can we stay with "rv"?

oh... you moved this var... ok, so leave it as res if you want

     // XXX in that case docshell hierarchyand SH hierarchy won't match.

wanna add the missing space here, while touching the code around it?

+    nsresult res = RemoveChildGroup(childAsDocloader);

can you name this rv? (or is "res" the style of this file?)

+    NS_WARN_IF_FALSE(aIndex >=0 && aIndex < mChildList.Count(), "index of
child element is out of range!");

wanna add a space after >=, and maybe wrap this so it fits into 80 cols?

+	 // XXXbz We could also pass |this| to nsIURILoader::Stop.  That will
+	 // just call Stop() on us as an nsIDocumentLoader... We need fewer
+	 // redundant apis!
+	 Stop();

if you ask me, we should put loadgroups into loadgroups and call
mLoadGroup->Cancel(NS_BINDING_ABORTED); anyway...

(guess we need stop anyway, via nsIWebNavigation)


there are a few more inconsistencies of "= do_QI(...)" vs "(do_QI(..))"

-	     srcContainer->GetChildAt(i, getter_AddRefs(srcChild));
+	     srcContainer->GetChildAt (i, getter_AddRefs(srcChild));

why this? doesn't seem to be file style, cf. GetChildCount a few lines above

-   if(aIID.Equals(NS_GET_IID(nsILinkHandler)))

why can you remove this?

oh I see... nsDocLoaderImpl implements GetInterface via QueryInterface... so I
guess you can ignore my above comment about the replacing of "return QI" with
"return doclaoderimpl::GI"; but I'm not sure it's wise to rely on this...

webshell/public/nsIWebShell.h
-  /**
-   * Return the nsIDocumentLoader associated with the WebShell.
-   */
-  NS_IMETHOD GetDocumentLoader(nsIDocumentLoader*& aResult) = 0;

you should change the IID

uriloader/exthandler/nsExternalHelperAppService.cpp
-  nsCOMPtr<nsIDocumentLoader> origContextLoader;
-  uriLoader->GetDocumentLoaderForContext(mWindowContext,
getter_AddRefs(origContextLoader));
+  nsCOMPtr<nsIDocumentLoader> origContextLoader =
+    do_GetInterface(mWindowContext);

great! it seems like uriLoader is unused now, can you remove it?

 NS_IMETHODIMP nsMsgWindow::GetRootDocShell(nsIDocShell * *aDocShell)
 {
-  if(!aDocShell)
-    return NS_ERROR_NULL_POINTER;

is this safe? this is mailnews after all...

mailnews/base/util/nsMsgMailNewsUrl.cpp
+	 m_loadGroup = do_GetInterface(docShell);

is this correctly indented? seems to me like it doesn't match with the } above

embedding/browser/webBrowser/nsWebBrowser.cpp
 NS_IMETHODIMP nsWebBrowser::GetParent(nsIDocShellTreeItem** aParent)
+   *aParent = nsnull;

hm, hope that's OK...


this patch is great stuff! thanks for doing this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Good catch with FireOnLocationChange.

I'd rather not have GetAsSupports an instance, so I can safely call it on null
pointers.

> wish we had a template array class for stuff like this

I do too... If I run into it again, it'll happen.

> Hm... looks like you are eliminating SetLoadCookie.

Yeah.  That bothered me too, a little. On the other hand, this is clearly
documented as an opaque pointer.  People using it really deserve what they get.
 I could fix the URLFetcher code, or I could just keep calling SetLoadCookie in
URILoader (and assert that it's the docshell in the docshell code).  Darin, do
you have an opinion here?  Calling it is kinda ugly, but less likely to break
people, I suppose...

> does this file contain _two_ #defines

It does.  I eliminated one of them and left the other so people have the define
after all.... ;)

> is that nullcheck needed?

I think so... callers just pass random stuff (some of it potentially coming from
JS) to this function, and CallQI will crash if passed null.

> loader.container == loader? nice :)

Yes.  I left the getter for backwards compat for now, but this method could
easily go away too....

> I don't like this... why not return null and override this in the docshell?

Because that would be more code?  This basically preserves the old codepath, and
means we don't have to worry about where the DOMWindow comes from, exactly.

> I don't understand this comment... can you explain?

The "that" is the GetContainer method immediately above this one... I'll clarify
the comment.

> hm, interesting... I assume you verified that this is safe :-)

Absolutely.  ;)

> I see that this code did that before too, but why?

Security reasons, I suspect... in any case, I was trying to not change existing
docshell functionality... this patch is scary enough as-is.

> hmm, you're inconsistent with constructor-style vs using = here...

I use the one more likely to fit in 80 chars, basically... and line-breaking in
the middle of the constructor-style one is annoying.

> what if the child does not have a parent?

Then we crash.  I needed a null-check here (eg kids of a destroyed docshell
would have no parent).  Thanks for catching this!

> can we stay with "rv"?

Not unless I update the rest of this function.  And there's no reason to touch
all that code.

> if you ask me, we should put loadgroups into loadgroups

Yes, but not this patch... ;)

> (guess we need stop anyway, via nsIWebNavigation)

That's a different Stop(); it takes an argument.  So we have 3 different ways to
call Stop() on a docshell -- via docloader, uriloader, and webnavigation.  Hence
"fewer redundant apis".

> hm, hope that's OK...

So do I, and from talking to danm it sounded like it ought to be... the parent
stuff should really not live on the same interface as what nsWebBrowser
implements, but refactoring the nsIDocShell* interfaces is a battle for a
separate patch.  ;)

I'll fix the other things you pointed out.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to biesi's comments

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 167899 [details] [diff] [review]
Updated to biesi's comments

 class nsDocLoaderImpl : public nsIDocumentLoader, 
...

Wanna drop "Impl" from the class name while you're at it, it's not like
nsDocLoader would conflict with anything, is it? And drop it from
GetAsDocLoaderImpl() too, maybe? Not a big deal, but I find it very unnecessary
to put "Impl" in classnames for no good reason...

- In nsDocLoaderImpl::DestroyChildren():

+  if (count > 0)
+  {
+    for (i=0; i < count; i++)
     ...
+    mChildList.Clear();

I guess I don't see the point of that if check, all it saves us is the for loop
that won't do anything, and a mChildList.Clear() call on an already empty list.

- In nsDocShell::IsFrame():

-    if (mParent) {
+    nsCOMPtr<nsIDocShellTreeItem> parent =
+	 do_QueryInterface(GetAsSupports(mParent));
+    if (parent) {
	 PRInt32 parentType = ~mItemType;	 // Not us
-	 mParent->GetItemType(&parentType);
+	 parent->GetItemType(&parentType);
	 if (parentType == mItemType)	 // This is a frame
	     return PR_TRUE;
     }

Couldn't this use GetSameTypeParent() to save us a bit of code duplication?

sr=jst

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'll make those changes.  I also made two more changes:

1) I added the following block of code to addChild:

    // Make sure we're not creating a loop in the docshell tree
    nsDocLoaderImpl* ancestor = this;
    do {
        if (childAsDocLoader == ancestor) {
            return NS_ERROR_ILLEGAL_VALUE;
        }
        ancestor = ancestor->GetParent();
    } while (ancestor);

  and added comments to nsIDocShellTreeNode to indicate that this exception
  would be thrown in this circumstance.

2) I fixed nsDocShell::GetInterface not to return NS_OK if it's setting the out
   param to null (which it could do in some cases).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to jst's sr comments

Comment 16

[Darin Fisher]

Comment on attachment 168169 [details] [diff] [review]
Updated to jst's sr comments

>Index: uriloader/base/nsIDocumentLoader.idl

>  * An nsIDocumentLoader is a component responsible for tracking groups of loads

nsIDocumentLoader is not a component; it's an interface ;-)


>+  readonly attribute nsILoadGroup loadGroup;
> 
>   readonly attribute nsIChannel documentChannel;

documentChannel == loadGroup.defaultLoadRequest, right?


care to document more of nsIDocumentLoader?


>Index: uriloader/base/nsDocLoader.h

>+    static nsresult AddDocLoaderAsChildToRoot(nsDocLoader* aDocLoader);

...ChildOfRoot perhaps?


>+    // Remove aChild from our childlist.  This nulls out the child's mParent
>+    // pointer.
>+    nsresult RemoveChildGroup(nsDocLoader *aChild);
>+    // Add aChild to our child list.  This will set aChild's mParent pointer to
>+    // |this|.
>+    nsresult AddChildGroup(nsDocLoader* aChild);

RemoveChildFromGroup / AddChildToGroup?  or maybe just Remove/AddChild?


>Index: uriloader/base/nsDocLoader.cpp

>-NS_INTERFACE_MAP_END
>+   if (aIID.Equals(kThisImplCID))
>+     foundInterface = NS_STATIC_CAST(nsIDocumentLoader *, this);
>+   else
>+NS_INTERFACE_MAP_END_THREADSAFE

you can just use NS_INTERFACE_MAP_END; there's nothing special about
NS_INTERFACE_MAP_END_THREADSAFE.  i think it was intended to be 
deprecated.  it's only AddRef and Release that have the concept of
threadsafe vs. non-threadsafe.


>+nsDocLoader::AddDocLoaderAsChildToRoot(nsDocLoader* aDocLoader)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIDocumentLoader> docLoaderService =
>+    do_GetService(NS_DOCUMENTLOADER_SERVICE_CONTRACTID, &rv);

any value in caching a pointer to the root docloader?


>+nsDocLoader::GetContainer(nsISupports** aResult)
> {
>+   NS_ADDREF(*aResult = NS_STATIC_CAST(nsIDocumentLoader*, this));
> 
>    return NS_OK;
> }

what does this mean anyways?  can we fix the callers to just use
the docloader as the container?


>Index: docshell/base/nsDocShell.h

>@@ -184,32 +184,33 @@ class nsDocShell : public nsIDocShell,
...
>+                   public nsDocLoader

it might be better to list nsDocLoader first in the inherits
from list.  that way you avoid creating thunks for any methods
that you override.


>+    NS_IMETHOD Stop() {
>+        // Need this here because otherwise nsIWebNavigation::Stop
>+        // overrides the docloader's Stop()
>+        return nsDocLoader::Stop();
>+    }

Hmm... is this going to be fragile across compilers?  Should we
rename the stop method on nsIDocumentLoader?


>+    // Need to implement (and forward) nsISecurityEventSink, because
>+    // nsIWebProgressListener has methods with identical names...
>+    NS_FORWARD_NSISECURITYEVENTSINK(nsDocLoader::)

I wonder if it wouldn't make sense to create helper classes for some 
of these interfaces with overlapping methods.  The same problem occurs
with nsPipe (where we have both nsIInputStream::close and
nsIOutputStream::close).


>Index: docshell/base/nsIDocShellTreeNode.idl

>+	alive only as long as it's referenced from outside the docshell tree.
>+    @throws NS_ERROR_ILLEGAL_VALUE if child corresponds to the same
>+            object as this treenode or an ancestor of this treenode.
>+    // XXXbz this should take an nsIDocShellTreeNode, I think.
> 	*/

indentation nits in this file.	looks like the existing code may use tabs.


>Index: docshell/base/nsDocShell.cpp

>+            prompter.swap((nsIPrompt*) *aSink);

this might not compile on MSVC 6.  i ran into trouble trying to
do this with similar code before.


nothing major, r=darin

Comment 17

[Boris Zbarsky [:bzbarsky]]

Darin, what are your thoughts on the load cookie stuff biesi raises in comment 9
(see my comments on it in comment 11)?  I'm thinking I should try to keep
calling SetLoadCookie for now and just pass the docshell.... and file a followup
to consider eliminating that call.

> documentChannel == loadGroup.defaultLoadRequest, right?

Yes.  I'll add some comments to nsIDocumentLoader.  Note that biesi and I were
thinking that the interface could really just work on going away.  The only
thing it provides at this point that's useful is the loadgroup, and we're not
sure we need a whole interface for that.

> ...ChildOfRoot perhaps?

Will do.

> RemoveChildFromGroup / AddChildToGroup?  or maybe just Remove/AddChild?

If I do AddChild/RemoveChild, I end up with shadowing from the
nsIDocShellTreeNode methods, so I'd have to declare the methods in docshell and
forward...  How about AddChildLoader/RemoveChildLoader?

> any value in caching a pointer to the root docloader?

Maybe.  This isn't really code that should run that often, so I didn't bother
for now.

> what does this mean anyways?  can we fix the callers to just use
> the docloader as the container?

I'm not sure what it means, to be truthful.  I just left it as-is for now, but I
plan to do a followup bug on this.

> it might be better to list nsDocLoader first in the inherits
> from list. 

OK.  That will change the nsISupports pointer that gets passed in when "this" is
passed to methods... but that really shouldn't matter, I guess.

> Hmm... is this going to be fragile across compilers?

No, because nsIWebNavigation::Stop has a different signature.  The problem is
that the method shadowing stuff shadows even methods with different signatures,
for some reason.

> Should we rename the stop method on nsIDocumentLoader?

I think we should work on removing the stop method on nsIDocumentLoader... ;)

> this might not compile on MSVC 6.

I'll watch out for that.  I assume we have a tinderbox that uses it?

I'll post a patch updated to these comments tomorrow (and check it in, once the
loadcookie issue is decided one way or another).

Comment 18

[Darin Fisher]

> Darin, what are your thoughts on the load cookie stuff biesi raises...

I don't think we should mess with frozen interfaces (nsIURLContentListener::
loadCookie I presume), even if they happen to be poorly documented.  It might be
wise to preserve as much of the behavior as possible.  Opaque or not, people may
have grown up depending on this interface for some unfathomable reason.


> How about AddChildLoader/RemoveChildLoader?

Yeah, that works.


> > any value in caching a pointer to the root docloader?
> 
> Maybe.  This isn't really code that should run that often, so I didn't bother
> for now.

OK


> > it might be better to list nsDocLoader first in the inherits
> > from list. 
> 
> OK.  That will change the nsISupports pointer that gets passed in when "this" 
> is passed to methods... but that really shouldn't matter, I guess.

/me hopes we don't have any weird dependencies on that.


> > Should we rename the stop method on nsIDocumentLoader?
> 
> I think we should work on removing the stop method on nsIDocumentLoader... ;)

Sounds good!


> > this might not compile on MSVC 6.
> 
> I'll watch out for that.  I assume we have a tinderbox that uses it?

Yes, of course!  The release builds also use VC6 IIRC.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to Darin's comments

I added a hack to deal with the loadcookie thing, but I do think it's safe to
remove it.  The only people who would be bitten are those using the unfrozen
nsIURILoader api and passing a context that's not a window to it.  I certainly
hope there aren't very many such people....  (the mailnews code in question was
simply copy-pasted from docshell, comments included; I will file a followup bug
to remove that code altogether).

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix more indent nits

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: For checkin

The last diff had an unwanted hunk in the exthandler; I removed that here.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in.  This actually increased codesize a tad, due to increases in the
sizes of vtables for docshell and webshell and more thunks (for the overridden
addref/release methods, etc).  Hopefully working on merging webshell into
docshell will help with that.

I filed bug 273760 on deciding what to do with the loadcookie issue.