Closed Bug 380454 Opened 13 years ago Closed 12 years ago

nsHTMLLinkElement fires events at unsafe times


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






(Reporter: sicking, Assigned: smaug)



(Whiteboard: [sg:critical])


(3 files, 2 obsolete files)

The DOMLinkAdded and DOMLinkRemoved events fired by nsHTMLLinkElement are fired during BindToTree and UnbindFromTree, which is not a safe time to fire events.

Could we fire these off an event? It'd mean that for DOMLinkRemoved the element would no longer be in the DOM, but I'm not sure if that matters?

Looking at lxr there are no hits for DOMLinkRemoved, but I suppose extensions might use it.
Jonas, how bad is this?
Flags: blocking1.9?
It's pretty bad. It should be quite possible to crash exploitably.
Assignee: general → jonas
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Whiteboard: [sg:critical]
I can look at this.
Jonas, take this back if you already have the patch.

I'll either make those events to be dispatching asynchronously or
synchronously after the topmost document update.
Assignee: jonas → Olli.Pettay
nsHTMLLinkElement::UnbindFromTree is difficult.
The event is dispatched before actual unbinding, so it gets propagated to
Actually, I think DOMLinkRemoved hasn't ever quite worked they it was suppose to.
The parent of the element is already unbound, so event propagation doesn't work.

I should have guessed. Code written by hyatt :(

The good thing is that making this all asynchronous should be less-regression risky.
Attached patch async events (obsolete) — Splinter Review
I hope the change to threadmanager is ok. Otherwise there is a crash
if one tries to dispatch runnable during shutdown.
Benjamin, asking your review for that.

nsPLDOMEvents bubble and are cancel-able, so similar to
the current DispatchTrustedEvent(..., PR_TRUE, PR_TRUE);
Attachment #298353 - Flags: review?(benjamin)
This is better.
Attachment #298353 - Attachment is obsolete: true
Attachment #298436 - Flags: review?(benjamin)
Attachment #298353 - Flags: review?(benjamin)
This makes GetCurrentThread to have similar check as what ::NewThread has
Attachment #298436 - Attachment is obsolete: true
Attachment #298437 - Flags: review?(benjamin)
Attachment #298436 - Flags: review?(benjamin)
Actually, if it already doesn't work, could we just rip it out? Originally I think that notification was added for the links toolbar (which dug out various meta information such as home page, next page etc), but that was pretty quickly backed out.
Comment on attachment 298437 [details] [diff] [review]
ugh, maybe this one

We could do that too, and bring it back if some extensions relies on the removal event.
Attachment #298437 - Flags: review?(benjamin)
The removal event does work, but apparently not the way it was supposed to.
If one adds event listener to the link element, then the listener
is called. But as far as I see the event isn't propagated to document.

Oh, wait, there is still ::LinkRemoved.
Though, chrome doesn't seem to have listeners for DOMLinkRemoved event.
Comment on attachment 298437 [details] [diff] [review]
ugh, maybe this one

This patch might still be safer, just in case some extension uses the removal event.
I forgot to remove bz' comments about unsafe event dispatch.
Those are in bind/unbind
Attachment #298437 - Flags: review?(benjamin)
Comment on attachment 298437 [details] [diff] [review]
ugh, maybe this one

Ugh... ok :-(
Attachment #298437 - Flags: review?(benjamin) → review+
Attachment #298437 - Flags: superreview?(jonas)
Attachment #298437 - Flags: superreview?(jonas) → superreview+
Had to back this out because few tests, which assume synchronous events, were 
added in bug 417143 few days ago.
There were also some other tests which might have failed.
Gavin, is there something in chrome which really expects DOMLinkAdded to happen
synchronously? I couldn't find anything and I'm right now changing the tests
to supports asynchronous DOMLinkAdded.
No, I don't think any chrome code relies on it being synchronous. The tests were likely written that way just because it's easier.
Attached patch -wSplinter Review
Comment on attachment 304231 [details] [diff] [review]
with test changed

Does the change to the test look ok?
Attachment #304231 - Flags: review?(
Attachment #304231 - Flags: review?( → review+
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #9)
> Actually, if it already doesn't work, could we just rip it out?

The async event added here pisses off the CC debug output too, since it called Unlink on the object yet it still lives (through the refptr in the dispatched event). Sure we can't just remove it?
if you really want to remove the "asynchronousness", one option would be to 
dispatch events after document update batch.
Note, both the events are asynchronous.
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
Group: core-security
You need to log in before you can comment on or make changes to this bug.