376562 - Remove nsIDocShellTreeItem::childOffset

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mats Palmgren (inactive)]

Followup from bug 162283

The number return by nsIDocShellTreeItem::GetChildOffset is not a valid
child index for its parent's child list (even though it's documented
as being that). This caused the crash in bug 162283.

This bug is for removing the remaining use in ESM::HandleAccessKey()
at least (which theoretically can cause a crash too, I think).

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This patch replaces nsIDocShellTreeItem::childOffset with
nsIDocShell::SetChildOffset since this offset is now only used internally
in nsDocShell.cpp.

I think we could take the ESM part of this patch on branches if we want
since it doesn't require the interface changes.

Comment 2

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

How much work would it be to get rid of mChildOffset ?

Comment 3

[Mats Palmgren (inactive)]

Not too hard, but risky.  The docshell could simply iterate the parent's
child list to figure out its index and then use that to index the history
items.  The problem is that history items are never removed but docshells
are, so they become out-of-sync.
Bug 162283 has a WIP that removes the corresponding history item so
that they would be in sync, but Boris thought that would be a risky change.
See bug 162283 comment 71 - 73.

Comment 4

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

Comment on attachment 260652 [details] [diff] [review]
Patch rev. 1


>     nsresult res = AddChildLoader(childAsDocLoader);
>     NS_ENSURE_SUCCESS(res, res);
>+    NS_ASSERTION(mChildList.Count() > 0,
>+                 "child list must not be empty after a successful add");
> 
>     // Set the child's index in the parent's children list 
>     // XXX What if the parent had different types of children?
>     // XXX in that case docshell hierarchy and SH hierarchy won't match.
>-    aChild->SetChildOffset(mChildList.Count() - 1);
>+    {
>+        nsCOMPtr<nsIDocShell> childDocShell = do_QueryInterface(aChild);
>+        if (childDocShell)
>+            childDocShell->SetChildOffset(mChildList.Count() - 1);
>+    }

Why adding the block?

>@@ -424,14 +424,19 @@ interface nsIDocShell : nsISupports
...
>+
>+  /**
>+   * Set the offset of this child in its container.
>+   */
>+  [noscript] void setChildOffset(in unsigned long offset);

This is ugly, but I don't know where else to put this.

r=me if you explain or fix the block-thingie.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 260652 [details] [diff] [review]
Patch rev. 1

> Why adding the block?

We don't need 'childDocShell' beyond that point.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260652 [details] [diff] [review]
Patch rev. 1

>Index: docshell/base/nsDocShell.h
>+    // with lower indicies

"indices"

>+    // as an index into the parent's child list (see bug 162283).  It MUST not
>+    // for that purpose.

"not be used for"?

>Index: content/events/src/nsEventStateManager.cpp
> // Note: for the in parameter aChildOffset,
> // -1 stands for not bubbling from the child docShell
> // 0 -- childCount - 1 stands for the child docShell's offset
> // which bubbles up the access key handling

That comment no longer has bearing on reality.  Re-document in terms of aBubbledFrom?  Probably just need to explain what the arg means.

sr=bzbarsky with those changes.

Comment 7

[Mats Palmgren (inactive)]

Fixed spelling.  Added a full-fledged doc comment on ESM::HandleAccessKey()
describing all params and how the method works.

Checked in to trunk at 2007-05-17 20:49 PDT.

-> FIXED