head.ready() never called if DOM and assets are loaded before the 300ms timer runs out #271

Closed
pavoleichler opened this Issue Nov 14, 2013 · 10 comments

Projects

None yet

2 participants

@pavoleichler

I have experienced the following issue on v1.0.1 and v1.0.2.
In Chrome on Windows the head.ready callbacks sometimes would not get executed and sometimes they did. Usually when simply navigating to a page they did not and on refresh they did.

I played around a bit with the head.js file and found out this happens when the DOM ready gets fired event and all assets get loaded before the 300ms timeout defined at the very end of file runs out.
This is what happens:

  1. when all assets got loaded, the isDomReady was still undefined
  2. when the DOM ready event was fired, the isHeadReady was still undefined
  3. then the timeout ran out and the isHeadReady was set to true, however it seems that no more attemps to call the handlers are made by head.js

Making the following quick change to the function called by setTimeout seems to fix the issue for me.

    setTimeout(function () {

        isHeadReady = true;
        each(queue, function (fn) {
            fn();
        });

        if (isDomReady && allLoaded()) {
            each(handlers.ALL, function (fn) {
                one(fn);
            });
        }

    }, 300);
@robert-hoffmann
Member

Thanks for finding this ..and me who thought i was done with the 1x branch ! :)

What's weird though, is that you say "Chrome", since the setTimeout() is actually only applied on browsers that do not support async loading, which Chrome does.

@pavoleichler

Hello Robert, no problem. :)

To me, it seems, that the setTimeout is called in any browser:
https://github.com/headjs/headjs/blob/master/src/1.0.0/load.js#L678

As it is the only place where isHeadReady is set to true, I guess it has to run in every browser. Otherwise, the condition after the DOM ready event would never evaluate to true:
https://github.com/headjs/headjs/blob/master/src/1.0.0/load.js#L661

I also noticed the following, but am unsure if it is a bug or a feature:

  1. handlers.ALL are called when all scripts are loaded and DOM ready event was already fired, here:
    https://github.com/headjs/headjs/blob/master/src/1.0.0/load.js#L346
  2. handlers.ALL are called whe the DOM event is fired, all scripts were loaded and the isHeadReady timeout ran out, here:
    https://github.com/headjs/headjs/blob/master/src/1.0.0/load.js#L661
    Thus, if the DOM gets ready first and then all scripts get loaded, the handlers may get called event if the isHeadReady is not true yet.
@robert-hoffmann
Member

Could you give me an example of usage for ready() & load() that caused this in chrome so i can run some tests here plz

@robert-hoffmann
Member

Still need an example, i could not replicate this

<!DOCTYPE html>
<html>
    <head>
        <meta charset="utf-8"/>
        <script>
            var start = performance.now();
        </script>
        <script src="load.js"></script>
    </head>
    <body>
        <script>        
            head.ready(document, function () {
                var end = performance.now();
                console.log("document.ready", (end-start));
            });

            head.ready(function () {
                var end = performance.now();
                console.log("load.ready", (end-start));
            });

            // local file which is actually empty, so very rapid to load
            head.js("chrome.js", function() {
                var end = performance.now();
                console.log("callback", (end - start));
            });
        </script>
    </body>
</html>

and in load.js

setTimeout(function () {
    var end = performance.now();
    console.log("setTimeout", (end - window.start));
    isHeadReady = true;
    each(queue, function (fn) {
        fn();
    });
}, 300);

console logs (chrome 32 & 33)

document.ready 50.0000000174623
callback 145.00000001862645
load.ready 146.00000000791624
setTimeout 529.9999999988358 
@pavoleichler

I have played a bit with your example and made the following minor changes.

  1. I have appended a link to the same HTML file in .
    I wasnt succesful replicating the issue by simply hitting F5, only by clicking this link. While I keep clicking the link, once in 10 or 20 tries, the JS scripts get loaded before the DOM is ready and the load.ready event is not fired.
  2. I have added a console.log('allLoaded') statement to the load.js file when all JS files are ready.
<html>
    <head>
        <script>
            var start = performance.now();
        </script>
        <script src="load.js"></script>        
    </head>
    <body>
        <script>            
            head.ready(document, function () {
                var end = performance.now();
                console.log("document.ready", (end-start));
            });

            head.ready(function () {
                var end = performance.now();
                console.log("load.ready", (end-start));
            });            

            // local file which is actually empty, so very rapid to load
            head.js("chrome.js");
        </script>

        <a href="index.html" >Link to self</a>

    </body>
</html>

and in load.js
(lines 334 - 355)

        loadAsset(asset, function () {

            ...

            // dom is ready & no assets are queued for loading
            // INFO: shouldn't we be doing the same test above ?
            if (allLoaded()){
                var end = performance.now();
                console.log("allLoaded", (end - window.start));
            }
            if (isDomReady && allLoaded()) {
                each(handlers.ALL, function (fn) {
                    one(fn);
                });
            }
        });

...

   setTimeout(function () {
        var end = performance.now();
        console.log("setTimeout", (end - window.start));
        isHeadReady = true;
        each(queue, function (fn) {
            fn();
        });
    }, 300);

Most of the time, the output looks like this:

document.ready 5.000000121071935 index.html:13
allLoaded 10.000000009313226 load.js:348
load.ready 11.000000173225999 index.html:18
setTimeout 304.0000000037253 

But once in a while I get this:

allLoaded 6.000000052154064 load.js:348
document.ready 10.999999940395355 index.html:13
setTimeout 304.9999999348074 
@robert-hoffmann
Member

cool, i'll test it that way and see if i can replicate it

..ah, one thing. Do you have cache headers properly set (check responses from server in network in console) ..maybe a race condition somewhere when cache is working and it's serving a cached copy. (load events are not the same when served from cache)

But you have to double check, cause by default chrome will deactivate cache when the console is open (settings).

@pavoleichler

Having a fresh mind in the morning I have come with a better suggestion to reproduce the issue. :) As it happens only when the JS files is loaded before the DOM gets ready, I have added an <ul> with thousand <li>s at the end of the html file. This postpones the DOM ready event a bit and now the allLoaded event is fired before the document.ready each time I click the link.

allLoaded 7.999999914318323 load.js:348
document.ready 44.999999925494194 index.html:13
setTimeout 304.0000000037253 load.js:684

However when hiting F5 / refresh everything still seems fine. When refreshing the page, my Chrome sends a request to the server for all resources to get a 304 Not modified. When I simply click the link, Chrome is not sending any requests at all, it simply uses the cached versions of the JS file. (In the Network panel, there is (from cache) in the size column.)

Here are my HTTP reponse headers for chrome.js, if it helps you:
on refresh

Connection:Keep-Alive
Date:Sat, 16 Nov 2013 12:02:50 GMT
ETag:"0-4eb3e8df03a17"
Keep-Alive:timeout=5, max=98
Server:Apache/2.4.4 (Win32) OpenSSL/0.9.8y PHP/5.4.16

on link click

Accept-Ranges:bytes
Connection:Keep-Alive
Content-Length:0
Content-Type:application/javascript
Date:Sat, 16 Nov 2013 11:46:43 GMT
ETag:"0-4eb3e8df03a17"
Keep-Alive:timeout=5, max=86
Last-Modified:Fri, 15 Nov 2013 22:16:48 GMT
Server:Apache/2.4.4 (Win32) OpenSSL/0.9.8y PHP/5.4.16
@robert-hoffmann
Member

I'm off for the WE, so won't have time to look further into this until Monday, but thanks for all the info. Hopefully with all that i'll be able to figure out whats up and make a fix !

@robert-hoffmann
Member

Did some fixes and ended up removing some old obsolete code which i think will fix this. Please test with this file and let me know if all is fine for you: http://headjs.com/dist/1.0.0/head.js

@pavoleichler

Hello Robert, I have tested the file and everything seems fine now, good job! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment