383930 - Replace document.popupNode with another mechanism

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

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.

Comment 1

[Neil Deakin]

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

Comment 2

[Neil Deakin]

Attachment: patch I wrote a while ago

Comment 3

[Neil Deakin]

Attachment: updated patch

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.

Comment 4

[[e-mail]]

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?

Comment 5

[Neil Deakin]

(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?

Comment 6

[[e-mail]]

(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.

Comment 7

[Neil Deakin]

(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.

Comment 8

[Neil Deakin]

(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.

Comment 9

[Neil Deakin]

Attachment: updated patch

Addresses the triggerContent comment and moves the access check to GetPopupNode/GetTooltipNode and adds tests that a child frame cannot access the node.

Comment 10

[[e-mail]]

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 11

[[e-mail]]

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.

Comment 12

[Neil Deakin]

Attachment: address above comments

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 14

[Neil Deakin]

Attachment: use sepearate GetLastTriggerNode methods

Comment 15

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/ed8906789d08

Comment 16

[[e-mail]]

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.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I think this needs docs.

Comment 18

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

So this landed without approval and wasn't a blocker either?

Comment 19

[Dão Gottwald [:dao]]

(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.

Comment 20

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

ah, ok.

Comment 21

[Wladimir Palant]

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.

Comment 22

[[e-mail]]

(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.

Comment 23

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/en/XUL/Method/openPopup
https://developer.mozilla.org/en/XUL/Property/triggerNode

And mentioned on https://developer.mozilla.org/en/XUL/menupopup and Firefox 4 for developers.