Closed
Bug 349465
Opened 18 years ago
Closed 18 years ago
Adopting a node should change the cx of all its event handlers
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: peterv)
References
Details
Attachments
(1 file)
10.54 KB,
patch
|
smaug
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
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.
![]() |
Reporter | |
Comment 1•18 years ago
|
||
We should really fix this for 1.9, imo. I'd love to fix it for branches too if that's remotely possible...
Flags: blocking1.9?
Updated•18 years ago
|
Flags: blocking1.8.0.8?
Assignee | ||
Comment 2•18 years ago
|
||
(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?
![]() |
Reporter | |
Comment 3•18 years ago
|
||
> 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.
Assignee | ||
Comment 4•18 years ago
|
||
(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.
![]() |
Reporter | |
Comment 5•18 years ago
|
||
Yeah, I agree that as things stand this is not entirely trivial. Otherwise I'd have just fixed it. :(
Assignee | ||
Comment 7•18 years ago
|
||
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)
Attachment #241065 -
Flags: review?
Assignee | ||
Comment 8•18 years ago
|
||
Attachment #241065 -
Flags: review? → review?(bzbarsky)
![]() |
Reporter | |
Comment 9•18 years ago
|
||
Yeah, that seems reasonable. Maybe have smaug review it?
Assignee | ||
Updated•18 years ago
|
Attachment #241065 -
Flags: superreview?(bugmail)
Attachment #241065 -
Flags: review?(bzbarsky)
Attachment #241065 -
Flags: review?(Olli.Pettay)
Attachment #241065 -
Flags: superreview?(bugmail) → superreview+
Updated•18 years ago
|
Attachment #241065 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 10•18 years ago
|
||
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•18 years ago
|
||
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))
Assignee | ||
Comment 12•18 years ago
|
||
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)?
Assignee | ||
Comment 13•18 years ago
|
||
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?
Looks like this got backed out for bug 355457, so reopening at least until it relands.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•18 years ago
|
||
Relanded.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 16•18 years ago
|
||
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.
Flags: blocking1.8.1.1?
Assignee | ||
Comment 17•18 years ago
|
||
(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•18 years ago
|
||
Need a branch patch
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Assignee | ||
Comment 19•18 years ago
|
||
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?
![]() |
Reporter | |
Comment 20•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
based on comments, not going to look at fixing this on branches
Flags: blocking1.8.1.1-
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9-
Flags: blocking1.8.0.9+
![]() |
Reporter | |
Updated•18 years ago
|
Flags: blocking1.9?
You need to log in
before you can comment on or make changes to this bug.
Description
•