Closed Bug 394600 Opened 13 years ago Closed 13 years ago

Address bz's followup comments to bug 279703

Categories

(Core :: XUL, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Depends on 1 open bug)

Details

(Whiteboard: [dbaron-1.9:RsCs])

Attachments

(1 file, 3 obsolete files)

Attached patch address comments (obsolete) — Splinter Review
No description provided.
Attachment #279280 - Flags: superreview?(bzbarsky)
Attachment #279280 - Flags: review?(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.
Attachment #279280 - Flags: superreview?(bzbarsky)
Attachment #279280 - Flags: superreview-
Attachment #279280 - Flags: review?(bzbarsky)
Attachment #279280 - Flags: review-
Requesting blocking, since some of the issues this is fixing are potential deleted-object accesses.
Flags: blocking1.9?
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?


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?
Assignee: nobody → enndeakin
Attachment #279280 - Attachment is obsolete: true
Attachment #281229 - Flags: superreview?(bzbarsky)
Attachment #281229 - Flags: review?(bzbarsky)
> 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?
> Are you thinking of specific case 

If we ever make all frames absolute containing blocks (as we plan to, long-term), for example.
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.
Attachment #281229 - Flags: superreview?(bzbarsky)
Attachment #281229 - Flags: superreview-
Attachment #281229 - Flags: review?(bzbarsky)
Attachment #281229 - Flags: review-
(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.
  
> 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.
(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.
Attached patch address extra comment (obsolete) — Splinter Review
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.
Attachment #281229 - Attachment is obsolete: true
Attachment #283047 - Flags: superreview?(bzbarsky)
Attachment #283047 - Flags: review?(bzbarsky)
Flags: blocking1.9? → blocking1.9+
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.
It would be good to run those test with XPCOM leak logging enabled; that should check issues with not deleting nsMenuChainItems.
Attached patch address issuesSplinter Review
> 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.
Attachment #283047 - Attachment is obsolete: true
Attachment #284937 - Flags: superreview?(bzbarsky)
Attachment #284937 - Flags: review?(bzbarsky)
Attachment #283047 - Flags: superreview?(bzbarsky)
Attachment #283047 - Flags: review?(bzbarsky)
Comment on attachment 284937 [details] [diff] [review]
address issues

Looks good.  Let's do it.
Attachment #284937 - Flags: superreview?(bzbarsky)
Attachment #284937 - Flags: superreview+
Attachment #284937 - Flags: review?(bzbarsky)
Attachment #284937 - Flags: review+
Whiteboard: [dbaron-1.9:RsCs]
Attachment #284937 - Flags: approval1.9?
Comment on attachment 284937 [details] [diff] [review]
address issues

a=drivers for after M9 freeze
Attachment #284937 - Flags: approval1.9? → approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
Depends on: CVE-2010-0183
Depends on: 563410
You need to log in before you can comment on or make changes to this bug.