We can have several DOMContentLoaded events with HTML Imports

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: Gabor Krizsanits (INACTIVE))

Tracking

unspecified
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
It seems we get a DOMContentLoaded event on window when the linked file is imported
(which means we can get several DOMContentLoaded events which is IMO dangerous for the web).

This happens if the first DOMContentLoaded was already sent before the linked file is imported. This can happen if there are a lot of script/css files to be loaded before the imported file. I managed to reproduce by simulating this behavior, inserting the link after the frist DOMContentLoaded. See http://everlong.org/mozilla/testcase-import/

I don't really know which pref I need to make rel=import work; this doesn't work on a fresh profile but works in a DEBUG Gaia profile.

This happens when I run the Gaia::SMS application inside Firefox (I can give more information in how to do this if you want) because we use `rel="import"` links for some time, with some JS to simulate what was implemented in bug 877072.
We certainly shouldn't be firing DOMContentLoaded on the window from import documents... Why are we?
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 2

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)
> We certainly shouldn't be firing DOMContentLoaded on the window from import
> documents... Why are we?

All the import documents share the same window, so I guess this is a side effect of that fact... and we should guard against this somehow. My problem is that I do depend on this event internally in the ImportLoader, so is there a way to prevent this event to bubble up further from the import document itself? I would just call naively stopPropagation on it from http://mxr.mozilla.org/mozilla-central/source/content/base/src/ImportManager.cpp#555 Does that sound like a sane idea?

The other problem is ofc then we might have the same issue for other events too, so maybe we should find a more generic way to prevent events of imported documents to bubble up further than the document itself...
Flags: needinfo?(gkrizsanits)

Comment 3

4 years ago
Share the same window? In which way?
Does imported document's .defaultView() return the Window object?

If you want to prevent events to not propagate from Document higher up, you want to change
nsDocument::PreHandleEvent. (but it still sounds weird that imported documents share the Window, does the spec really define it that way? and if so, why wouldn't event propagate to Window?)
(Assignee)

Comment 4

4 years ago
(In reply to Olli Pettay [:smaug] from comment #3)
> Share the same window? In which way?
> Does imported document's .defaultView() return the Window object?

According to the spec html imports do not have their own browser context, but window returns the window of the master and scripts in imported documents running in the context of the window, but defaultView is null (I have not yet landed that patch). So I would say, in a weird way...

Comment 5

4 years ago
I don't see http://w3c.github.io/webcomponents/spec/imports/ mentioning 'window' anywhere.

Running scripts in the context of the main document's window doesn't really mean "sharing" to me.
That is closer to what normal data documents do, but we'd just enable script execution by using
something like
if (IsImportDoc() && (nsIScriptGlobalObject* sgo = GetScopeObject())) {
  ...use sgo to as the scope.
}
(Assignee)

Comment 6

4 years ago
(In reply to Olli Pettay [:smaug] from comment #5)
> I don't see http://w3c.github.io/webcomponents/spec/imports/ mentioning
> 'window' anywhere.

Yeah, it's not over specified at the moment... https://www.w3.org/Bugs/Public/show_bug.cgi?id=26682

> 
> Running scripts in the context of the main document's window doesn't really
> mean "sharing" to me.

Probably "sharing" was a poor choice of word, it's certainly very vague and probably misleading.

> That is closer to what normal data documents do, but we'd just enable script
> execution by using

Exactly.

> something like
> if (IsImportDoc() && (nsIScriptGlobalObject* sgo = GetScopeObject())) {
>   ...use sgo to as the scope.
> }

What I did is changing getWindowInternal to return the window of the master for imports http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#4709, that might have been a bad move... Maybe we should just do something less generic like what you suggest.
(Assignee)

Comment 7

4 years ago
In particular the main reason I did return window from  GetWindowInternal was this:
1. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#295

but also various stuff in nsContentUtils like:
2. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3687

For 1., I could instead return null from GetWindowInternal for imports and do the same logic logally instead.

What should I do with 2. ?

AdoptNode is also an interesting case... mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7489

Comment 8

4 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> In particular the main reason I did return window from  GetWindowInternal
> was this:
> 1.
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsScriptLoader.cpp#295
That should explicitly check whether we have a window or are in import document
(or nsIDocument could perhaps have some helper method of in-browsing-context-or-import-document)


> 2.
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsContentUtils.cpp#3687
That is not supposed to work for import documents. Why would one use an import document or element in it to dispatch events to chrome?
(Assignee)

Comment 9

4 years ago
(In reply to Olli Pettay [:smaug] from comment #8)
> > 2.
> > http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> > nsContentUtils.cpp#3687
> That is not supposed to work for import documents. Why would one use an
> import document or element in it to dispatch events to chrome?

Not sure yet. I can imagine more and more XBL stuff to be replaced by web components in the future. In particular I can imagine chrome only components that are used internally that might need this. But for now I think we're safe to turn it off, and get back to it once we have an actual use case for it.
(Assignee)

Comment 10

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f4662d9027be
Assignee: nobody → gkrizsanits
(Assignee)

Comment 11

4 years ago
Created attachment 8504624 [details] [diff] [review]
returning null from GetWindowInternall for imports
Attachment #8504624 - Flags: review?(bugs)
Comment on attachment 8504624 [details] [diff] [review]
returning null from GetWindowInternall for imports

Oh, MasterDocument() returns something even in case of data documents.
The comment in nsIDocument is just a bit wrong, since there might not be
any browsing context.

Couldn't you just always do
nsCOMPtr<nsPIDOMWindow> window = 
  do_QueryInterface(mDocument->MasterDocument()->GetWindow());
No need for the if.
If we don't have master document, MasterDocument() returns just the document
itself. And in case of data documents GetWindow() will return null.

With that, r+



(Not related to this bug, but do import documents inherit loadgroup from the master? The code above takes loadgroup from document, not from master, so just wondering whether that works.)
Attachment #8504624 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

4 years ago
(In reply to Olli Pettay [:smaug] from comment #12)
> No need for the if.

Ah yeah, indeed, thanks for noticing it.

> 
> 
> (Not related to this bug, but do import documents inherit loadgroup from the
> master? The code above takes loadgroup from document, not from master, so
> just wondering whether that works.)

We create a new loadgorup for each import in the masters loadgroup. But I'm really
glad you asked it because I checked the code and instead of adding the new loudgroup
to the masters I create it accidentally in the import parents loadgroup which is a bug.
https://hg.mozilla.org/mozilla-central/rev/c7e87e494687
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1082946
You need to log in before you can comment on or make changes to this bug.