Replace document.popupNode with another mechanism

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

({dev-doc-complete})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

12 years ago
Use a separate event object for the four XUL popup events. This would allow the popup event to hold the original popupNode instead of all of the globals on XUL documents.

Updated

11 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
(Assignee)

Comment 1

10 years ago
Changing this bug into one about replacing document.popupNode, which is per-top-level-window global and arbitrarily accurate.

Instead, I propose to have a property on popups, popup.triggerNode to get the element that triggered the popup opening. This would mean:

- the value would remain even if other popups were open
- it wasn't global
- it can be set automatically for most popups
Summary: Create separate popupshowing/etc events → Replace document.popupNode with another mechanism
(Assignee)

Comment 2

9 years ago
Posted patch patch I wrote a while ago (obsolete) — Splinter Review
(Assignee)

Updated

9 years ago
Depends on: 560653
Blocks: 552341
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

9 years ago
Posted patch updated patch (obsolete) — Splinter Review
This patch adds popup.triggerNode which is valid while the popup is open. document.popupNode may still be and is cleared when the popup is closed to prevent the leak in bug 552341. Embedding cases might still leak if they set popupNode, but I don't think that is much of an issue.
Assignee: nobody → enndeakin
Attachment #439236 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #459807 - Flags: review?(neil)
Comment on attachment 459807 [details] [diff] [review]
updated patch

A few questions from skimming the patch...

>+  // returns the parent menupopup, if any
>+  nsMenuFrame* GetParentMenu() {
>+    nsIFrame* parent = GetParent();
>+    if (parent && parent->GetType() == nsGkAtoms::menuFrame) {
>+      return static_cast<nsMenuFrame *>(parent);
>+    }
>+    return nsnull;
>+  }
Doesn't do_QueryFrame let you do this already?

>+  *aTriggerContent = nsnull;
>+  if (aEvent) {
Why not null-check aTriggerContent instead? Saves passing dummy pointers.

>-    // which node is our tooltip on?
>-    nsCOMPtr<nsIDOMXULDocument> xulDoc(do_QueryInterface(currentTooltip->GetDocument()));
>-    if (!xulDoc)     // remotely possible someone could have 
>-      return NS_OK;  // removed tooltip from dom while it was open
>-    nsCOMPtr<nsIDOMNode> tooltipNode;
>-    xulDoc->TrustedGetTooltipNode (getter_AddRefs(tooltipNode));
...
>+    nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
>+    if (pm) {
>+      nsCOMPtr<nsIDOMNode> tooltipNode;
>+      pm->GetLastTriggerNode(PR_TRUE, currentTooltip->GetCurrentDoc(),
>+                             getter_AddRefs(tooltipNode));
Doesn't this suffer from the same bug that TrustedGetTooltipNode was invented to work around, i.e. this could get called with content code on the stack?
(Assignee)

Comment 5

9 years ago
(In reply to comment #4)

> >+  // returns the parent menupopup, if any
> >+  nsMenuFrame* GetParentMenu() {
> >+    nsIFrame* parent = GetParent();
> >+    if (parent && parent->GetType() == nsGkAtoms::menuFrame) {
> >+      return static_cast<nsMenuFrame *>(parent);
> >+    }
> >+    return nsnull;
> >+  }
> Doesn't do_QueryFrame let you do this already?

I could. The rest of the popup code uses the pattern above as do_QueryFrame didn't exist as the time. Is do_QueryFrame preferred or faster?


> Doesn't this suffer from the same bug that TrustedGetTooltipNode was invented
> to work around, i.e. this could get called with content code on the stack?

That code is called from a mouseout event listener. What situation where content code was on the stack could occur or cause a problem here?
(In reply to comment #5)
> (In reply to comment #4)
> The rest of the popup code uses the pattern above as do_QueryFrame
> didn't exist as the time. Is do_QueryFrame preferred or faster?
I don't really know, roc or dbaron could probably advise you best.

> > Doesn't this suffer from the same bug that TrustedGetTooltipNode was invented
> > to work around, i.e. this could get called with content code on the stack?
> That code is called from a mouseout event listener. What situation where
> content code was on the stack could occur or cause a problem here?
I don't understand bug 319434 comment 4 to know whether it still applies.
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)

> > > Doesn't this suffer from the same bug that TrustedGetTooltipNode was invented
> > > to work around, i.e. this could get called with content code on the stack?
> > That code is called from a mouseout event listener. What situation where
> > content code was on the stack could occur or cause a problem here?
> I don't understand bug 319434 comment 4 to know whether it still applies.

I think that comment was referring to a call from content script which caused a flush/reflow and prevented a call to GetPopupNode from succeeding.

This patch only checks CanCallerAccess(node) from C++ in three cases, two of which should be checked with CanCallerAccess and the other from a mouseout event listener.

Regardless, I think I'll move the CanCallerAccess check to the two callers that need it.
(Assignee)

Comment 8

9 years ago
(In reply to comment #6)
> > The rest of the popup code uses the pattern above as do_QueryFrame
> > didn't exist as the time. Is do_QueryFrame preferred or faster?
> I don't really know, roc or dbaron could probably advise you best.

The menu/popup frames don't implement the right macros to get do_QueryFrame to work, so I'm going to fix the do_QueryFrame issue all at once in bug 582719.
(Assignee)

Comment 9

9 years ago
Posted patch updated patch (obsolete) — Splinter Review
Addresses the triggerContent comment and moves the access check to GetPopupNode/GetTooltipNode and adds tests that a child frame cannot access the node.
Attachment #459807 - Attachment is obsolete: true
Attachment #463127 - Flags: review?(neil)
Attachment #459807 - Flags: review?(neil)
Comment on attachment 463127 [details] [diff] [review]
updated patch

>-  virtual void GetPopupNode(nsIDOMNode** aNode) = 0;
>+  virtual already_AddRefed<nsIDOMNode> GetPopupNode() = 0;
Looking good; the only nit that obviously stood out was that this looks as if it would be better off as a simple nsIDOMNode* return value.
Comment on attachment 463127 [details] [diff] [review]
updated patch

>-// 2e26a297-6e40-41c1-81c9-7306571f955e
>+// 426C1B56-E38A-435E-B291-BE1557F2A0A2
My personal preference is for lowercase UUIDs. In fact, some Mac users go as far as asking firebot for UUIDs as they can't remember how to lowercase text.
Attachment #463127 - Flags: review?(neil) → review+
(Assignee)

Comment 12

9 years ago
Posted patch address above comments (obsolete) — Splinter Review
Attachment #463127 - Attachment is obsolete: true
Attachment #463175 - Flags: superreview?(roc)
+  already_AddRefed<nsIDOMNode> GetLastTriggerNode(PRBool aIsTooltip, nsIDocument* aDocument);

bool parameters suck. How about just having two methods? They can call an internal method with a bool parameter if you like.
(Assignee)

Comment 14

9 years ago
Attachment #463175 - Attachment is obsolete: true
Attachment #463637 - Flags: superreview?(roc)
Attachment #463175 - Flags: superreview?(roc)
Attachment #463637 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 15

9 years ago
http://hg.mozilla.org/mozilla-central/rev/ed8906789d08
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
This broke the copy link (cmd_copyLink) and various copy image (cmd_copyImage, cmd_copyImageLocation, cmd_copyImageContents) commands. They only try to retrieve the popup node from the window root, and not the popup manager.

Updated

9 years ago
Depends on: 585841

Updated

9 years ago
Blocks: 586408

Updated

9 years ago
Blocks: 587343
I think this needs docs.
Keywords: dev-doc-needed
Depends on: 588070
So this landed without approval and wasn't a blocker either?
(In reply to comment #18)
> So this landed without approval and wasn't a blocker either?

I suppose blocking was implicitly inherited from bug 552341.
ah, ok.

Comment 21

9 years ago
This introduced a behavior change that is probably not a bug but should be noted in the developer docs I think. Previously when a submenu of the context menu was opened the popupshowing event handler of this submenu could still access document.popupNode. Now it is null however. Adblock Plus was implicitly relying on the old behavior which made a (trivial) fix necessary.
(In reply to comment #21)
> This introduced a behavior change that is probably not a bug
I believe that change is a bug and have attached a patch to bug 586408.
No longer blocks: 586408
Depends on: 586408
Depends on: 627065
You need to log in before you can comment on or make changes to this bug.