Closed
Bug 380454
Opened 17 years ago
Closed 16 years ago
nsHTMLLinkElement fires events at unsafe times
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: smaug)
References
Details
(Whiteboard: [sg:critical])
Attachments
(3 files, 2 obsolete files)
3.98 KB,
patch
|
benjamin
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
13.52 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
11.50 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•16 years ago
|
||
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]
Assignee | ||
Comment 3•16 years ago
|
||
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
Assignee | ||
Comment 4•16 years ago
|
||
nsHTMLLinkElement::UnbindFromTree is difficult. The event is dispatched before actual unbinding, so it gets propagated to document.
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
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)
Assignee | ||
Comment 7•16 years ago
|
||
This is better.
Attachment #298353 -
Attachment is obsolete: true
Attachment #298436 -
Flags: review?(benjamin)
Attachment #298353 -
Flags: review?(benjamin)
Assignee | ||
Comment 8•16 years ago
|
||
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
Attachment #298436 -
Attachment is obsolete: true
Attachment #298437 -
Flags: review?(benjamin)
Attachment #298436 -
Flags: review?(benjamin)
Reporter | ||
Comment 9•16 years ago
|
||
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.
Assignee | ||
Comment 10•16 years ago
|
||
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)
Assignee | ||
Comment 11•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
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.
Assignee | ||
Comment 13•16 years ago
|
||
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 14•16 years ago
|
||
Comment on attachment 298437 [details] [diff] [review] ugh, maybe this one Ugh... ok :-(
Attachment #298437 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #298437 -
Flags: superreview?(jonas)
Assignee | ||
Comment 15•16 years ago
|
||
Jonas?
Reporter | ||
Updated•16 years ago
|
Attachment #298437 -
Flags: superreview?(jonas) → superreview+
Assignee | ||
Comment 16•16 years ago
|
||
Had to back this out because few tests, which assume synchronous events, were added in bug 417143 few days ago.
Assignee | ||
Comment 17•16 years 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.
Comment 18•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
Assignee | ||
Comment 20•16 years ago
|
||
Assignee | ||
Comment 21•16 years ago
|
||
Comment on attachment 304231 [details] [diff] [review] with test changed Does the change to the test look ok?
Attachment #304231 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Attachment #304231 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 419747
(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?
Assignee | ||
Comment 23•16 years ago
|
||
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.
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•