349465 - Adopting a node should change the cx of all its event handlers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Here's what happens:

1)  We parse the XBL.
2)  We clone the <img> node when installing XBL anonymous content.
3)  Cloning sets the on* attributes on the new node.
4)  Setting on* attributes calls into
    nsEventListenerManager::AddScriptEventListener
5)  This looks for an nsIScriptGlobalObject to use.
6)  The document's script global is still null, since the owner doc at this
    point is the XBL document.  So we fall back to the "get JS context from the
    stack, or safe JS context" thing.  Luckily, there's no chrome on the stack,
    so we get the hidden window context (safe context).
7)  We store this context in our listener.
8)  Later on, we check whether we can execute the listener and pass the context
    to the security check.  The security manager uses this to get to the
    docshell.
9)  It finds the hidden window.

Then later, when we actually put the node in the bound document, we don't change the cx in the JS event listeners.  This causes problems.  I'm adding code to nsJSEnvironment to work around those problems, but that code should really be removed once this is fixed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should really fix this for 1.9, imo.  I'd love to fix it for branches too if that's remotely possible...

Comment 2

[Peter Van der Beken [:peterv]]

(In reply to comment #0)
> 2)  We clone the <img> node when installing XBL anonymous content.

> 6)  The document's script global is still null, since the owner doc at this
>     point is the XBL document.

Would that part be fixed by the fix for bug 347523?

Regarding adoptNode, I guess we want to call nsEventListenerManager::AddScriptEventListener on the eventlistener manager of the new document? Do we need to do something to remove stuff from the old document? Anything else we need to do?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Would that part be fixed by the fix for bug 347523?

Yes, at which point different steps to reproduce would be needed.  ;)

> I guess we want to call nsEventListenerManager::AddScriptEventListener on the
> eventlistener manager of the new document?

Er... event listener managers are per-node.  We need to somehow change the cx for all the script event listeners in the node's existing listener manager.

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to comment #3)
> Er... event listener managers are per-node.

Right, I keep making this mistake even though deep down I know they are per-node :-/. Although for some listeners we use another listener manager (GetEventListenerManagerForAttr).

> We need to somehow change the cx
> for all the script event listeners in the node's existing listener manager.

I don't see how we can currently do that easily, we don't have a list of all the script event listeners. We could loop over the attributes when adopting (checking for event listener names) and call AddScriptEventListener again, but that's not very efficient.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, I agree that as things stand this is not entirely trivial.  Otherwise I'd have just fixed it.  :(

Comment 6

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1

So do we want to do this for now? (if you don't have time to review, just let me know what you think of the general approach)

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 241065 [details] [diff] [review]
v1

See comment 7.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, that seems reasonable.  Maybe have smaug review it?

Comment 10

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

Hmm, I think I missed MaybeAddPopupListener(attr)
That is called in nsXULElement::AddListenerFor, but not 
in nsXULElement::RecompileScriptEventListeners().
So BindToTree doesn't set popuplisteners anymore, right?

Comment 11

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

Comment on attachment 241065 [details] [diff] [review]
v1


>+        nsIAtom *attr = name->Atom();
>+        if (!IsEventHandler(attr)) {
>+            continue;
>+        }
>+
>+        nsAutoString value;
>+        GetAttr(kNameSpaceID_None, attr, value);
>+        AddScriptEventListener(attr, value, PR_TRUE);


Should you just call here AddListenerFor


>+
>+            if (!IsEventHandler(attr)) {
>+                continue;
>+            }
>+
>+            nsAutoString value;
>+            GetAttr(kNameSpaceID_None, attr, value);
>+            AddScriptEventListener(attr, value, PR_TRUE);

And here too.
AddListenerFor sets the popuplistener and then checks if (!IsEventHandler(attr))

Comment 12

[Peter Van der Beken [:peterv]]

Those should be handled by the call to TransferOrDeleteAllPropertiesFor. I don't see anything in the popup listener that would be affected by the ownerDocument changing (apart from the property)?

Comment 13

[Peter Van der Beken [:peterv]]

There is one issue though: nsDOMEventRTTearoff::AddEventListener checks !nsContentUtils::IsChromeDoc(mContent->GetOwnerDoc()) to determine if the listener wants untrusted events. If the type of ownerDocument changes we need to toggle that flag. I do think that the old code had the same problem though, it would just add a new listener with the flag toggled, so the old listener would stick around with the wrong flag, right? If that's the case then I propose we just file a follow-up bug, I don't suppose there's an easy way to toggle the flag on a registered listener?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like this got backed out for bug 355457, so reopening at least until it relands.

Comment 15

[Peter Van der Beken [:peterv]]

Relanded.

Comment 16

[Daniel Veditz [:dveditz]]

What got relanded, the same patch? Or did you do something to address the Ts hit?

Is this reasonable to land on the branches? seems like the kind of thing we'd want to fix.

Comment 17

[Peter Van der Beken [:peterv]]

(In reply to comment #16)
> What got relanded, the same patch?

Yes.

> Or did you do something to address the Ts
> hit?

No, since backing out didn't affect Ts.

> Is this reasonable to land on the branches? seems like the kind of thing we'd
> want to fix.

Part of the code that this fixes isn't on the branch, so would need a different patch.

Comment 18

[Daniel Veditz [:dveditz]]

Need a branch patch

Comment 19

[Peter Van der Beken [:peterv]]

Actually, I'm not sure we should port this to the branch. Branch doesn't have adoptNode, so the only places that would need fixup are the BindToTree implementations where we change the ownerDocument. That's a bit risky, I didn't do that on the trunk because the trunk will not change the ownerDocument in BindToTree anymore after 47903 lands. Bz, what do you think?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yeah, we'd have to do it in BindToTree on branches.  If we can do it safely, great.  If not, probably not worth worrying about...

Comment 21

[Reed Loden [:reed]]

As this is blocking1.8.1.1+, please either request approval1.8.1.1 on the current patch or, if needed, attach a branch version of the patch and request approval1.8.1.1 on it.

The same holds true for blocking1.8.0.9+.

Comment 22

[Mike Connor [:mconnor]]

based on comments, not going to look at fixing this on branches