394600 - Address bz's followup comments to bug 279703

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Neil Deakin]

Attachment: address comments

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 279280 [details] [diff] [review]
address comments

This doesn't address the very first comment in bug 279703 comment 100.

>Index: layout/xul/base/src/nsMenuFrame.cpp
> nsMenuFrame::InsertFrames(nsIAtom*        aListName,

Should we only do the moving to the popup list if !aListName or something?  Same for AppendFrames.

>Index: layout/xul/base/src/nsMenuFrame.h
>+  // the menuitem. If the menu should be opened, this frame will be returned.
>   nsMenuFrame* Enter();

And if not?  null?  Document?

>+  // initialize mPopupFrame to the first popup frame within aChildList 
>+  nsIFrame* SetPopupFrame(nsIFrame* aChildList);

And the return value means what?

>Index: layout/xul/base/src/nsMenuPopupFrame.h
>+  // just pass the call down to the current menu, if any. If a current menu
>+  // should be opened as a result, this method should return the frame for
>+  // that menu. Also, calling Enter will reset the current incremental search

And if nothing should be opened it should return null?  Document?

>Index: layout/xul/base/src/nsXULPopupManager.cpp
> nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)
>+      if (!weakFrame.IsAlive())
>+        break;

Won't that possibly prevent hiding of some popups?  I don't think we want that...

Why are we starting with the top visible menu anyway?  Why not just start with mCurrentMenu and loop while it's true, always hiding the thing in mCurrentMenu and detaching as we go?

Of course we could get screwed if the script in question opens more popups from the relevant handlers...  Not sre what we can do about that.

Similar issues with panels.

>@@ -754,17 +769,21 @@ nsXULPopupManager::ExecuteMenu(nsIConten
>+      nsWeakFrame weakFrame(frame);

This has similar issues.  Perhaps we should build up a list of nsWeakFrames to hide or something, then delete all the items, then hide them all?  Or some other approach?  As things stand, we might not hide everything we need to.

>@@ -1109,38 +1128,43 @@ nsXULPopupManager::PopupDestroyed(nsMenu

Same issues here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Requesting blocking, since some of the issues this is fixing are potential deleted-object accesses.

Comment 3

[Neil Deakin]

OK, I'm looking at building an array of nsWeakFrames and then hiding them all afterwards. This is the code I am using.

void nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)
{
  nsTArray<nsWeakFrame> popupsToHide;

  nsMenuChainItem* item = GetTopVisibleMenu();
  while (item) {
    nsMenuChainItem* parent = item->GetParent();
    if (item->Content()->GetOwnerDoc() == aDocument) {
      nsMenuPopupFrame* frame = item->Frame();
      item->Detach(&mCurrentMenu);
      delete item;
      popupsToHide.AppendElement(frame);
    }
    item = parent;
  }

  for (PRUint32 f = 0; f < popupsToHide.Length(); f++) {
    if (popupsToHide[f].IsAlive()) {
      nsMenuPopupFrame* frame =
        static_cast<nsMenuPopupFrame *>(popupsToHide[f].GetFrame());
      frame->HidePopup(PR_TRUE, ePopupInvisible);
    }
  }

I have a test which when hiding the first popup, uses a mutation listener to hide the second popup, causing the frame to be destroyed. Yet, the weak frame still seems to be alive even though the frame has been destroyed. That is, popupsToHide[1].IsAlive returns true yet it's frame is destroyed.

Maybe I'm missing something obvious here?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm... The weakframe implementation depends on weakframes not just moving in memory, as far as I can see, and nsTArray will realloc its buffer as needed, right?  Does preallocating the nsTArray to the right size help?

Comment 5

[Neil Deakin]

Attachment: address comments, and add some tests

Comment 6

[Neil Deakin]

> Should we only do the moving to the popup list if !aListName or something? Same for AppendFrames.

I didn't address this one. Are you thinking of specific case that might be handled in an odd way?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Are you thinking of specific case 

If we ever make all frames absolute containing blocks (as we plan to, long-term), for example.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281229 [details] [diff] [review]
address comments, and add some tests

This STILL doesn't address the very first comment in bug 279703 comment 100.

Please actually address all the review comments, then ask for review?  The other approach means that reviews take a really long time as I carefully compare every single comment I've made to the patch instead of just reading the patch code.

Comment 9

[Neil Deakin]

(In reply to comment #8)
> (From update of attachment 281229 [details] [diff] [review])
> This STILL doesn't address the very first comment in bug 279703 comment 100.
> 

Which comment are you referring to? The one about the curly braces? That code doesn't exist currently so the comment doesn't apply.
  

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That code doesn't exist currently

What's http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/content/src/nsXULPopupListener.cpp&rev=1.153&mark=219-220#219 then?

Seriously, please check over the comments and make sure they're all addressed?  I'm in a position where I can probably do one and only one more pass over this stuff in the next 6-7 months, given that it's likely to take me several hours to do it.

Comment 11

[Neil Deakin]

(In reply to comment #10)
> > That code doesn't exist currently
> 
> What's
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/content/src/nsXULPopupListener.cpp&rev=1.153&mark=219-220#219
> then?
> 

OK, sorry about that, I must have been looking at the code wrong when I created the latest patch. I'll look through the comments again and make sure and create a new patch.

Comment 12

[Neil Deakin]

Attachment: address extra comment

The only change from the previous patch is the addition of the curly braces. I looked through the patch again and have addressed all the comments in the patch or in comments in the bug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283047 [details] [diff] [review]
address extra comment

>Index: layout/xul/base/public/nsXULPopupManager.h

>This function
>+  // can cause style changes and frames to be destroyed.

s/frames to be destroyed/frame destruction/ perhaps?

>Index: layout/xul/base/src/nsMenuFrame.cpp
>+nsMenuFrame::SetInitialChildList(nsIAtom*        aListName,

This needs an aListName check like AppendFrames/InsertFrames, right?

>Index: layout/xul/base/src/nsXULPopupManager.cpp
>+nsXULPopupManager::HidePopupsInList(const 

>+  nsTArray<nsWeakFrame> weakPopups(aFrames.Length());

What do we want to do if the array allocation fails?

>+    *wframe = aFrames[f];

Right now it looks like we write to random memory near address 0 right here in that case (and probably crash).

> nsXULPopupManager::HidePopupsInDocument(nsIDocument* aDocument)

>+    if (item->Frame()->PopupState() != ePopupInvisible &&
>+        (aDocument && item->Content()->GetOwnerDoc() == aDocument)) {

Take out the extra parentheses?

>@@ -747,31 +785,34 @@ nsXULPopupManager::ExecuteMenu(nsIConten
>@@ -1089,62 +1130,57 @@ nsXULPopupManager::PopupDestroyed(nsMenu

I don't see us deleting the items we detach here.  Did I miss it?

The rest looks great.

Comment 14

[Boris Zbarsky [:bzbarsky]]

It would be good to run those test with XPCOM leak logging enabled; that should check issues with not deleting nsMenuChainItems.

Comment 15

[Neil Deakin]

Attachment: address issues

> What do we want to do if the array allocation fails?

I added a null check for this, and just skip appending the element if allocation fails. I don't think there's much else that can be done here.

> I don't see us deleting the items we detach here.  Did I miss it?

Added an extra delete

I also tested with the leak tools. With the extra delete, everything is being cleaned up ok.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 284937 [details] [diff] [review]
address issues

Looks good.  Let's do it.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 284937 [details] [diff] [review]
address issues

a=drivers for after M9 freeze