688580 - deferred script runs after DOMContentLoaded (use http: to repro, ok when loaded as file:)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[al_9x]

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

___________________________________
<script>
function handler(e) {
 console.log(e.type + ':' + e.target);
}
document.addEventListener('load', handler, true);
document.addEventListener('DOMContentLoaded', handler, false);
</script>
<script defer src="script.js"></script>
___________________________________

[14:28:01.433] GET defer.htm [HTTP/1.1 200 OK 0ms]
[14:28:01.586] DOMContentLoaded:[object HTMLDocument] @ defer.htm:3
[14:28:01.641] GET script.js [HTTP/1.1 200 OK 0ms]
[14:28:01.663] external @ script.js:1
[14:28:01.671] load:[object HTMLScriptElement] @ defer.htm:3

Comment 1

[al_9x]

This may be difficult to repro, Borris can't, so you probably shouldn't clear the hardware and os for now

Comment 2

[al_9x]

The test page is on a localhost web server, if I load it as file://, the script runs before DOMContentLoaded

Comment 3

[al_9x]

Can anyone confirm with http?

Comment 4

[Boris]

http://dl.dropbox.com/u/5134541/defertest.html

This is a link where you could test it. Expected result is:

afterjquery/load/DOMContentLoaded/load document/

FF 7.0.1 gives:

DOMContentLoaded/afterjquery/load/load document/

FF nightly (build ID 10.0a1 (2011-11-05)) gives:

DOMContentLoaded/afterjquery/load/load document/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Noted Gaia might be affected by this bug because the way l10n.js is written. See bug 831228 comment 13.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #6)
> Noted Gaia might be affected by this bug because the way l10n.js is written.
> See bug 831228 comment 13.

Never mind, I just packaged the test case on comment 4 and load it on the phone and B2G Desktop; app:// does not affected by this bug.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is this still happening? It sounds like a pretty bad problem if so.

Henri, is this something you could look at. Sounds like it's a regression from bug 518104.

Comment 9

[Henri Sivonen (:hsivonen)]

Looking.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> Never mind, I just packaged the test case on comment 4 and load it on the
> phone and B2G Desktop; app:// does not affected by this bug.

On a second thought, Gaia is affected by this bug in DEBUG=1 mode (the mode enabling Gaia to be able to be developed in Nightly).

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Fix that makes various seemingly unrelated tests orange

It turns out that our XSLT impl. is so broken it doesn't fire DOMContentLoaded at all, so I decided to leave fixing XSLT out of the scope of this patch.

As for HTML and XML, this patch seems to fix the bug, but it adds an even loops spin even when there are no defer scripts. Various seemingly unrelated tests go orange. Yay.

I expect I need to change the patch to make this less of a fix and omit an event loop spin where the spec requires one in the case where there are no defer scripts.

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Make DOMContentLoaded happen later when there's <script defer src> present

This does not fix the pre-existing bug that XSLT-generated docs don't fire DOMContentLoaded at all! This intentionally violates the spec by not spinning the event loop between the readystatechange to "interactive" and DOMContentLoaded when there are no deferred scripts, because doing so would make too many things go orange (bug 883828). This adds a pref to undo this change so that we have the option to ship a hotfix if this breaks something big time on the release channel.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> app:// does not affected by this bug.

That seems like a bug in itself, since it suggests that app:// does synchronous main-thread IO.

(We really should make sure no URL schemes do synchronous IO.)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 763520 [details] [diff] [review]
Make DOMContentLoaded happen later when there's <script defer src> present

This looks oddly complicated.
Couldn't we just increase some counter in a document if async DOMContentLoaded is needed, and decrease when the thing needed it is done. 
If the counter drops to 0, 
    nsRefPtr<nsIRunnable> ev =
      NS_NewRunnableMethod(this, &nsDocument::DispatchContentLoadedEvents);
    NS_DispatchToCurrentThread(ev);
like in nsDocument::EndLoad().
EndLoad() would also check if the counter is zero and if not, wouldn't
dispatch the event at all.

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: Fix bitrot

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 8343017 [details] [diff] [review]
Fix bitrot

(In reply to Olli Pettay [:smaug] from comment #14)
> This looks oddly complicated.

Sadly, yes. (Still, I'd prefer to get this landed, since the longer this remains unfixed, the more probable it is that sites start relying on the bogus behavior.)

> Couldn't we just increase some counter in a document if async
> DOMContentLoaded is needed, and decrease when the thing needed it is done. 
> If the counter drops to 0, 
>     nsRefPtr<nsIRunnable> ev =
>       NS_NewRunnableMethod(this, &nsDocument::DispatchContentLoadedEvents);
>     NS_DispatchToCurrentThread(ev);
> like in nsDocument::EndLoad().
> EndLoad() would also check if the counter is zero and if not, wouldn't
> dispatch the event at all.

This seems brittle, since DOMContentLoaded and the load event are rather tightly coupled with parsing and script execution, but we'd be pretending that they are independent things and stuff just happens to work right when counters that you don't see in the script loader code are poked at the right way.

Rerequesting review, since it seems to me that the proposed alternative would be worse than this patch (which admittedly is sad).

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Henri Sivonen (:hsivonen) from comment #16)

> This seems brittle, since DOMContentLoaded and the load event are rather
> tightly coupled with parsing and script execution,
which load event are you talking about here? load event fired on window object certainly isn't bound to
parsing in anyway. (well, it never happens before parsing has finished)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Henri Sivonen (:hsivonen) from comment #16)
> Rerequesting review, since it seems to me that the proposed alternative
> would be worse than this patch (which admittedly is sad).

How so? What I'm proposing is closer to how we block load event to fire too early.
We have nsIDocument::BlockOnload() and nsIDocument::UnblockOnload().
In order to keep these APIs simple and consistent, I think adding
nsIDocument::BlockContentLoaded() and nsIDocument::UnblockContentLoaded() would make sense.
No need to special case nsScriptLoader etc so much.

Comment 19

[Matt Basta [:basta]]

Are there any updates on this?

Comment 20

[Henri Sivonen (:hsivonen)]

(In reply to Matt Basta [:basta] from comment #19)
> Are there any updates on this?

There's a patch that works, but the reviewer didn't agree with the approach. Realistically, I won't have the time to try re-implementing this is a different way any time soon. If you need this soon, I think it would be reasonable to check if schedule pressure has weight against the r- considerations.

Regarding comment 18, I think the BlockOnload/UnblockOnload is remarkably bad and I think we shouldn't introduce similar machinery for DOMContentLoaded if we can help it.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Henri, Olli, Doug, Andrew, Jonas,

Could we have this resolved in a reasonable timeframe? Firefox cannot stay incompatible with the spec indefinitely, and this bug seriously stop Gaia developers from creating shared libraries that work consistently.

Without this bug, DOMContentLoaded will not fire on event listeners attached in a <script defer>. A script must execute immediately in the deferred script (and advertise with "this script must loaded with <script defer>"), or it has to loaded undeferred in order to catch the event. Neither is optimal, and we ended up with inconsistent launch strategy and behavior in _every_ Gaia apps.

The fact that this bug doesn't repro on app:// doesn't make this bug less serious; for day-to-day Gaia development, we rely on DEBUG=1 profiles to run Gaia in Firefox, loaded from a localhost http server. We need the two environments to be consistent.

I don't understand why we can sit on this for years. It's not that we block this out of any policy concern like WEBP or H264...

Comment 22

[Matt Basta [:basta]]

As a temporary workaround, you can place a data URI script tag with the defer attribute after your other scripts to fire a synthetic DCL event. It's a huge hack, but if you need code to fire at the right time and this bug is blocking you, that'll do it.

Comment 23

[Doug Turner (:dougt)]

> I don't understand why we can sit on this for years. It's not that we block this out of any policy concern like WEBP or H264...

Limited resources and priorities, my friend.  It looks like we've fumbled a bit here to.  I am going to assign to jst who is responsible for the DOM and I am going to bother him tomorrow in person about this.  No promises of when it can be fixed or what needs to be done next, but there will be a response soon.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Doug Turner (:dougt) from comment #23)
> Limited resources and priorities, my friend.  It looks like we've fumbled a
> bit here to.  I am going to assign to jst who is responsible for the DOM and
> I am going to bother him tomorrow in person about this.  No promises of when
> it can be fixed or what needs to be done next, but there will be a response
> soon.

Thanks.

Comment 25

[Henri Sivonen (:hsivonen)]

I'm back from paternity leave, so working on this again would now be more realistic. I still think that this stuff belongs in nsScriptLoader and spreading the interactions to nsIDocument and giving an appearance of loose coupling when the stuff is actually *very* tightly coupled would not be an improvement compared to the attached patch that works but got r-. (I think blocking/unblocking the load event is not at all a positive role model.) I'd still prefer revising the review status instead of revising the approach of trying to fit the code as close to nsScriptLoader as possible. Looking at the patch again, I still think the approach I took is pretty reasonable given the constraints that make it infeasible to do away with the notion of having to request async behavior in some cases instead of having it in all cases.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: block/unblock approach

So I was thinking something like this.
Didn't try to fix DOMContentLoaded for xslt with this setup, but should be doable.
https://tbpl.mozilla.org/?tree=Try&rev=9d8b0d5cb06f

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Oh, lovely, try didn't like that patch at all... or I pushed some other patch.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Better luck with this https://tbpl.mozilla.org/?tree=Try&rev=366f1e7e7a82

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

That didn't work either. Some odd errors, not related to the patch.
Maybe this https://tbpl.mozilla.org/?tree=Try&rev=34b929e24d78

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: block/unblock approach (missing test file added)

https://tbpl.mozilla.org/?tree=Try&rev=a16c4f0e49f7

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> > app:// does not affected by this bug.
> 
> That seems like a bug in itself, since it suggests that app:// does
> synchronous main-thread IO.
> 
> (We really should make sure no URL schemes do synchronous IO.)

Re-read the bug I realize this can be something important worthy of investigating. If this is true, Gecko is probably the biggest contributor to app launch time when we launch a packaged app, from a ZIP package, on the poor flash memory. And we have been looking at the wrong place (Gaia) for a long time.

Should I file a unconfirmed bug for people to investigate? How do we investigate this?

Comment 32

[Boris Zbarsky [:bzbarsky]]

app:// normally just delegates to jar:, no?

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Attachment: defer_smaug_5.diff

This relies on the parser calling either both BeginLoad/EndLoad or neither, 
and the parser wasn't doing that so had to change it a bit.
Good thing is that the change removes couple of assertions we have had.

Comment 34

[Henri Sivonen (:hsivonen)]

Comment on attachment 8401807 [details] [diff] [review]
defer_smaug_5.diff

Review of attachment 8401807 [details] [diff] [review]:
-----------------------------------------------------------------

This completely removes the one-way increasing enum that I expect to save some later pain compared to the block/unblock approach. Please at least approximate having such an enum by making the block/unblock stuff assert on readyState. With that, I'd r+.

::: content/base/public/nsIDocument.h
@@ +1324,5 @@
>    virtual void UnblockOnload(bool aFireSync) = 0;
>  
> +  void BlockDOMContentLoaded()
> +  {
> +    ++mBlockDOMContentLoaded;

Please assert that readyState has an expected value when this method gets called.

::: content/base/src/nsDocument.cpp
@@ +4934,5 @@
> +
> +void
> +nsDocument::UnblockDOMContentLoaded()
> +{
> +  MOZ_ASSERT(mBlockDOMContentLoaded);

Please also assert that readyState has the value that's appropriate at the time of this method call.

@@ +4951,5 @@
>  }
>  
>  void
> +nsDocument::UnblockDOMContentLoadedAsync()
> +{

Again, please assert that readyState has the value that's appropriate at the time of this method call.

::: content/base/src/nsScriptLoader.cpp
@@ +1490,5 @@
> +
> +void
> +nsScriptLoader::AddDeferRequest(nsScriptLoadRequest* aRequest)
> +{
> +  mDeferRequests.AppendElement(aRequest);

Why is the request simply added without checking if we are past the point where adding defer scripts is legitimate?

@@ +1499,5 @@
> +  }
> +}
> +
> +void
> +nsScriptLoader::MaybeRemovedDeferRequests()

Why is the name of this method plural? Can we ever remove more than one defer request at a time?

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Henri Sivonen (:hsivonen) from comment #34)
> > +  void BlockDOMContentLoaded()
> > +  {
> > +    ++mBlockDOMContentLoaded;
> 
> Please assert that readyState has an expected value when this method gets
> called.
The idea is that you can call the API even after the document as been loaded. It's just no-op.
Similar to load blocker.

> > +nsScriptLoader::AddDeferRequest(nsScriptLoadRequest* aRequest)
> > +{
> > +  mDeferRequests.AppendElement(aRequest);
> 
> Why is the request simply added without checking if we are past the point
> where adding defer scripts is legitimate?
Because it doesn't matter whether we're past that point.


> > +nsScriptLoader::MaybeRemovedDeferRequests()
> 
> Why is the name of this method plural? Can we ever remove more than one
> defer request at a time?
Yes. there is .Clear() call for the array.

Comment 36

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #35)
> (In reply to Henri Sivonen (:hsivonen) from comment #34)
> > > +  void BlockDOMContentLoaded()
> > > +  {
> > > +    ++mBlockDOMContentLoaded;
> > 
> > Please assert that readyState has an expected value when this method gets
> > called.
> The idea is that you can call the API even after the document as been
> loaded. It's just no-op.

No-ops like that make it harder to reason about what went wrong when things do go wrong. Leaving things implicit like this and not noticing that things are wrong leads to bugs like bug 779959, which require major changes to fix after the fact. Bugs like that and the test suite growing to depend on them are basically what's still keeping the remains of the old HTML parser around just for about:blank.

As I said on IRC when we discussed this, I think it's very bad not to have an enum that tracks the lifecycle, progresses in one direction only, can be asserted on and can be inspected in a debugger.

> > > +nsScriptLoader::AddDeferRequest(nsScriptLoadRequest* aRequest)
> > > +{
> > > +  mDeferRequests.AppendElement(aRequest);
> > 
> > Why is the request simply added without checking if we are past the point
> > where adding defer scripts is legitimate?
> Because it doesn't matter whether we're past that point.

It can easily matter for debugging later.

Comment 37

[Shian-Yow Wu [:swu]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #31)
> (In reply to Henri Sivonen (:hsivonen) from comment #13)
> > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> > > app:// does not affected by this bug.
> > 
> > That seems like a bug in itself, since it suggests that app:// does
> > synchronous main-thread IO.
> > 
> > (We really should make sure no URL schemes do synchronous IO.)
> 
> Re-read the bug I realize this can be something important worthy of
> investigating. If this is true, Gecko is probably the biggest contributor to
> app launch time when we launch a packaged app, from a ZIP package, on the
> poor flash memory. And we have been looking at the wrong place (Gaia) for a
> long time.
> 
> Should I file a unconfirmed bug for people to investigate? How do we
> investigate this?

I am not sure about previous implementation.  But the lastest libjar uses AsyncRead() to read local files.
http://dxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#417

This indicates that the current app:// is doing asynchronous main-thread IO.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Henri Sivonen (:hsivonen) from comment #36) 
> > > > +nsScriptLoader::AddDeferRequest(nsScriptLoadRequest* aRequest)
> > > > +{
> > > > +  mDeferRequests.AppendElement(aRequest);
> > > 
> > > Why is the request simply added without checking if we are past the point
> > > where adding defer scripts is legitimate?
> > Because it doesn't matter whether we're past that point.
> 
> It can easily matter for debugging later.
I'm just keeping the existing behavior.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v8

some simplifications, adds assert.

Comment 40

[Henri Sivonen (:hsivonen)]

Comment on attachment 8403230 [details] [diff] [review]
v8

Review of attachment 8403230 [details] [diff] [review]:
-----------------------------------------------------------------

r+ regardless of the one suggested added assertion in order to make progress, but I'd prefer you add one assertions still:

::: content/base/src/nsScriptLoader.cpp
@@ +1493,5 @@
> +{
> +  mDeferRequests.AppendElement(aRequest);
> +  if (mDeferEnabled && mDeferRequests.Length() == 1 && mDocument &&
> +      !mBlockingDOMContentLoaded) {
> +    mBlockingDOMContentLoaded = true;

Please assert on mDocument's readyState here.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +assertion

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02876730f55f

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android 4.0 debug mochitest-7 perma-fail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/902ce09a4dba

https://tbpl.mozilla.org/php/getParsedLog.php?id=37568923&tree=Mozilla-Inbound

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

That patch ended up moving files in M* tests, and previously hidden test started to run on
Android 4 debug.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Attachment: disabling the failing test on Android

https://tbpl.mozilla.org/?tree=Try&rev=5f93094c09ed

Comment 46

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9acc076ad18f

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9acc076ad18f