Closed Bug 380454 Opened 13 years ago Closed 12 years ago
HTMLLink Element fires events at unsafe times
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?
It's pretty bad. It should be quite possible to crash exploitably.
Assignee: general → jonas
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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 document.
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.
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);
This is better.
This makes GetCurrentThread to have similar check as what ::NewThread has http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/threads/nsThreadManager.cpp&rev=3.3&mark=211-215#211
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.
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. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/public/nsILink.h&rev=1.9&mark=89-95#89 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
Comment on attachment 298437 [details] [diff] [review] ugh, maybe this one Ugh... ok :-(
Attachment #298437 - Flags: review?(benjamin) → review+
Attachment #298437 - Flags: superreview?(jonas)
12 years ago
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.
Comment on attachment 304231 [details] [diff] [review] with test changed Does the change to the test look ok?
Attachment #304231 - Flags: review?(gavin.sharp)
Attachment #304231 - Flags: review?(gavin.sharp) → review+
Status: NEW → RESOLVED
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.
12 years ago
Depends on: 425551
You need to log in before you can comment on or make changes to this bug.