Open Bug 696885 Opened 13 years ago Updated 2 years ago

DOMContentLoaded attached on appcontent, fires for an orphaned doc, already replaced in the dom tree

Categories

(Core :: DOM: Events, defect, P5)

Product:
Core
Component:
DOM: Events
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
UNCONFIRMED

People

(Reporter: al_9x, Unassigned)

Details

https://groups.google.com/group/mozilla.dev.tech.dom/browse_thread/thread/af26484d4ca83036

This scenario occurs when clicking on google web search cache links, but here's a synthetic test:
_______________________________________________________
top1.html:

<iframe src="iframe.html"></iframe>

iframe.html:

<script>
parent.location = 'top2.htm';
location = 'about:blank'
</script>
______________________________________________

DOMContentLoaded is attached by an extension (optimizegoogle) overlay.

it fires for iframe.html which at that point appears to be orphaned
(In reply to al_9x from comment #0)
> 
> it fires for iframe.html which at that point appears to be orphaned

document.location (event.target.location) is null
event.target is iframe.html (but it's already been unhooked and replaced):
event.target.defaultView.document is about:blank
Henri, do we fire DOMContentLoaded unconditionally when the parser terminates?
(In reply to Boris Zbarsky (:bz) from comment #2)
> Henri, do we fire DOMContentLoaded unconditionally when the parser
> terminates?

Yes.

nsHtml5TreeOpExecutor::DidBuildModel (and in the old world, HTMLContentSink::DidBuildModel) call nsDocument::EndLoad unconditionally.
In that case looks like everything is behaving as it should here.
(In reply to Boris Zbarsky (:bz) from comment #4)
> In that case looks like everything is behaving as it should here.

Are you saying that DOMContentLoaded for a doc that's no longer part of the DOM, makes sense?
DOM document itself is the root of a "DOM".
Yep.  All DOMContentLoaded means is "parser is don".  It can fire as a result of the user hitting the stop button partway through the page load, for example.
Parser being done for a document that's no longer in the tree is irrelevant.  Events only makes sense for current nodes, not past nodes.
> Parser being done for a document that's no longer in the tree is irrelevant.

Why?  For example, the responseXML of an XMLHttpRequest is "no longer in the tree" (and in fact is never in the tree), right?  Similar for documents created via DOMImplementation.

> Events only makes sense for current nodes, not past nodes.

If you have a reference to it, it's not a "past node".
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
(In reply to Boris Zbarsky (:bz) from comment #9)
> > Parser being done for a document that's no longer in the tree is irrelevant.
> 
> Why?  For example, the responseXML of an XMLHttpRequest is "no longer in the
> tree" (and in fact is never in the tree), right?  Similar for documents
> created via DOMImplementation.
> 

that's straw man, we are clearly talking about documents that are meant to be part of the DOM tree (rooted somewhere in chrome)

> > Events only makes sense for current nodes, not past nodes.
> 
> If you have a reference to it, it's not a "past node".

I don't have a reference to it, the event handler was attached to appcontent.  It is a past node because it is no longer DOM tree.  Event bubbling makes sense for the nodes currently in the tree.  How is it logical for an event from a disconnected node bubble up to appcontent?
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
> we are clearly talking about documents that are meant to be part of the DOM tree

The document itself doesn't care what's "meant".  It just fires an event when it's done parsing.

If your handler is not attached to the document itself, then it may make sense to cut off the event propagation at the window if it's not the current inner.  Olli?
(In reply to Boris Zbarsky (:bz) from comment #11)
> > we are clearly talking about documents that are meant to be part of the DOM tree
> 
> The document itself doesn't care what's "meant".  It just fires an event
> when it's done parsing.

When a doc is unhooked from the tree (window), that is the end of its life, logically it no longer exists.  That it still fires an event is an implementation artifact, no events should be delivered from it.

> 
> If your handler is not attached to the document itself, then it may make
> sense to cut off the event propagation at the window if it's not the current
> inner.  Olli?

As I mentioned in the newsgroup thread, a handler attached to the document itself in content, is already not invoked. That makes sense, there should be no events from a dead doc, regardless of where they were attached.
> When a doc is unhooked from the tree (window), that is the end of its life

No, it's really not.  For example, documents in the bfcache are thus unhooked but are most definitely not at the end of their life and very definitely exist.  Again, if you have a reference to the document it would all be there with all its elements and such.
(In reply to Boris Zbarsky (:bz) from comment #13)
> > When a doc is unhooked from the tree (window), that is the end of its life
> 
> No, it's really not.  For example, documents in the bfcache are thus
> unhooked but are most definitely not at the end of their life and very
> definitely exist. 

Should docs in bfcache fire events that propagate up the current/live dom tree?  That's what's at issue here.
Events can certainly be fired on docs in bfcache, and event listeners attached to those documents would get them.

I agree that propagating the event to chrome is probably not desirable.
There are few different cases. What if DOMContentLoaded is already dispatched and some event listener
in the page causes a new page to be loaded? In that case capturing listeners in chrome have
already run, should also listeners in bubble phase run?
(In reply to Boris Zbarsky (:bz) from comment #15)
> Events can certainly be fired on docs in bfcache, and event listeners
> attached to those documents would get them.
> 

Can you give an example of that?  I thought that bfcashed docs are supposed to be "frozen" so nothing should run in them, is that not the case?
Nothing runs in the bfcached documents, but you may still have a pointer to them. And if
an event is dispatched to such document, it ends up to chrome.
That is a case we could easily change though, since in that case we know before event dispatch
that the document is not the current document in the outer window and event target chain could stop
to the inner window of the bfcached document.
(In reply to Olli Pettay [:smaug] from comment #16)
> There are few different cases. What if DOMContentLoaded is already
> dispatched and some event listener
> in the page causes a new page to be loaded? In that case capturing listeners
> in chrome have
> already run, should also listeners in bubble phase run?

No, after a doc is unhooked, no listeners should run.  I don't think anyone desires or even anticipates encountering docs in this state.  If such listeners run they will never do anything useful and only waste resources and produce bugs.
Actually, I see the behavior that event listeners don't currently run in the bfcached documents
as a bug, If you have a document and it has listener, those listeners should run when an event
is dispatched. That is what the spec says.
Because of certain security related issues we've disabled event handling in bfcached documents.

But, whether or not events propagate from a bcached document to chrome is all Gecko internal
thing, and we could change that.
(In reply to Olli Pettay [:smaug] from comment #20)
> Actually, I see the behavior that event listeners don't currently run in the
> bfcached documents
> as a bug, If you have a document and it has listener, those listeners should
> run when an event
> is dispatched. That is what the spec says.

Link?  A bfcached document is supposed to be frozen, it's cached data, it should not be running.  Content scripts are not written with disconnected bfcached documents in mind, if you let them run not only will it be a waste of resources (all these cached docs doing stuff), but they will fail in subtle ways because they will expect the document to be part of the current dom tree.
(In reply to al_9x from comment #21)
> not only will it be a waste
> of resources (all these cached docs doing stuff)

I guess if only events bfcached docs can receive are those programmatically dispatched by content (or extension) code (is that so?), resources are not a concern.

I mentioned resources because initially I imagined flurry of background activity in numerous bfcached docs with events enabled.
(In reply to al_9x from comment #21)
> (In reply to Olli Pettay [:smaug] from comment #20) 
> Link? 
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html
(In reply to Olli Pettay [:smaug] from comment #23)
> (In reply to al_9x from comment #21)
> > (In reply to Olli Pettay [:smaug] from comment #20) 
> > Link? 
> http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html

That's a big document, please be more specific, where does it mention bfcached documents needing to run listeners?
bfcache is Gecko internal thing, but documents have an API defined in that spec. And the spec is
clear what should happen if you have listener added to some node and you dispatch an event.
> please be more specific, where does it mention bfcached documents needing to run
> listeners?

Any node with a listener attached needs to have that listener run when the relevant event is dispatched.  Doesn't matter what sort of document it's in.

If script holds a ref to a document and that document is unloaded or goes into bfcache, it's not like that document disappears: script is holding a reference to it.  It can still create elements from that document, import them into other documents, import elements into this document, fire events at it, etc.  It's just an in-memory DOM not being shown.  Plenty of those around, as I pointed out in comment 9.  The spec is very clear on all this; it simply doesn't make any exceptions for such documents.

Again, that's all separate from what should happen with event propagation to chrome.
Ok, I understand, the key factor here is some non gecko code keeping a reference to the doc, then whether it's bfcached or not is immaterial.  However, I was more interested in the case where the doc is bfcached with no references to it kept, because that scenario is closer to this bug.  A bfcached doc, with no one pocking at it, will and should remain completely frozen (no code, no events), right?
No obvious guarantees of that, but probably yes.
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.