541079 - [HTML5][Patch] dom/tests/mochitest/ajax/offline/test_bypass.html fails

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

dom/tests/mochitest/ajax/offline/test_bypass.html fails with the HTML5 parser enabled.

Comment 1

[Henri Sivonen (:hsivonen)]

dom/tests/mochitest/ajax/offline/test_foreign.html fails, too.

Comment 2

[Honza Bambas (:mayhemer)]

Is there a bug for the HTML5 parser stuff? Are there patches I can apply locally and then debug these tests?

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Flip the pref

(In reply to comment #2)
> Is there a bug for the HTML5 parser stuff? Are there patches I can apply
> locally and then debug these tests?

To reproduce, the html5.enable pref needs to be true (see patch). When I apply the patch to the current trunk, I see the failure running dom/tests/mochitest/ajax/offline/test_bypass.html individually locally or among other test on the tryserver.

Comment 4

[Honza Bambas (:mayhemer)]

Thanks for the info.

As for test_bypass.html:

You are performing eTreeOpProcessOfflineManifest operation in nsHtml5TreeOpExecutor::Flush later then you are loading the scripts using the speculative loader.  eTreeOpProcessOfflineManifest associates the document with the application cache defined by the manifest= attribute.  It must be done before any loads associated with this document have to start.

Http channels loading the scripts are searching an application cache object in the document that invoked the load (it does GetCallback(nsIApplicationCacheContainer*) that leads to nsDocShell::GetInterface for that iid).  The manifest defining the cache for the failing test is prohibiting namespace1/script.js to load because it has not been defined in the manifest.  If the channel would found the application cache 
in its docshell, that script would be prevented from load.  But the channel believes that there is no application cache and normally loads the script.  

This will break offline applications in off-line mode.



As for test_foreign.html:

We do not get applicationCache.oncached in foreign2.html (sub load of test_foreign.html).  I didn't investigate why.  If you do not have any ideas I will do that.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to comment #4)
> You are performing eTreeOpProcessOfflineManifest operation in
> nsHtml5TreeOpExecutor::Flush later then you are loading the scripts using the
> speculative loader.  eTreeOpProcessOfflineManifest associates the document with
> the application cache defined by the manifest= attribute.  It must be done
> before any loads associated with this document have to start.

Thank you for investigating this.

My first choice would be making cache manifests also go to the speculative load fast track. Is there any reason against fixing this that way? The second alternative would be blocking speculative loads until the tree op for the root element has run.

> As for test_foreign.html:
> 
> We do not get applicationCache.oncached in foreign2.html (sub load of
> test_foreign.html).  I didn't investigate why.  If you do not have any ideas I
> will do that.

There are other cases where events don't fire into iframes into the test suite with the HTML5 parser even though the event do fire on the Web outside Mochitest. This issue might be a duplicate of bug 541050 and bug 539896. I so far have no idea why the event dispatch doesn't work all the way inside the test suite. In those two bugs, the event dispatch mechanism starts normally, but JS event handlers don't seem to run.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #5)
> My first choice would be making cache manifests also go to the speculative load
> fast track. Is there any reason against fixing this that way? The second
> alternative would be blocking speculative loads until the tree op for the root
> element has run.

The first solution will probably not associate the document with the cache and we actually do not need to run the update (load the manifest) but just to tell the document it is associated with an application cache as soon as possible (i.e. sooner any sub-load happens).  Also, the update process should start after the document loaded (after the onload handler was called) because the update invokes events on window.applicationCache object that should not be fired sooner then onload was.  And what more, window.applicationCache would be null anyway until we do not take manifest= attribute into account.

The second solution seems to be more appropriate but I presume you don't like it.  But, as stated in [1] the application cache selection algorithm (that is responsible for association of an application cache with a document) happens in the parser during parsing of the html element.  There is nothing said nor specified about a speculative load in relation to the application cache, so I suggest to block speculative loads until we know there is any associated cache (an associated manifest).  This should be known very soon, so there should be no high performance hit.  You may freely do pre-loads on any resources referenced before the html element.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#parser-appcache

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #6)
> (In reply to comment #5)
> > My first choice would be making cache manifests also go to the speculative load
> > fast track. Is there any reason against fixing this that way? The second
> > alternative would be blocking speculative loads until the tree op for the root
> > element has run.
> 
> The first solution will probably not associate the document with the cache and
> we actually do not need to run the update (load the manifest) but just to tell
> the document it is associated with an application cache as soon as possible
> (i.e. sooner any sub-load happens).

The speculative load requests are processed in the order the parser thread posted them, so if the cache manifest association was put onto that same side channel, it would happen before any scripts. Also, since there can be no scripts before the HTML element, the association wouldn't really be speculative but always a sure thing despite being on the speculative side channel for runnable ordering.

> Also, the update process should start
> after the document loaded (after the onload handler was called) because the
> update invokes events on window.applicationCache object that should not be
> fired sooner then onload was.  And what more, window.applicationCache would be
> null anyway until we do not take manifest= attribute into account.

Does the code actually want to see the HTML element with the manifest attribute in the DOM? Would the code work if it got told that a given URL is the manifest URL before the HTML element exists?

> The second solution seems to be more appropriate but I presume you don't like
> it.  But, as stated in [1] the application cache selection algorithm (that is
> responsible for association of an application cache with a document) happens in
> the parser during parsing of the html element.  There is nothing said nor
> specified about a speculative load in relation to the application cache, so I
> suggest to block speculative loads until we know there is any associated cache
> (an associated manifest).  This should be known very soon, so there should be
> no high performance hit.

What would be the observable difference of running the cache selection algorithm when the document object exists but the root element doesn't compared to deferring it to the moment when the root element exists?

The <base> element can't participate in complicating the URL absolutization at either point. And xml:base isn't available in the HTML serialization, so it can't break things, either.

> You may freely do pre-loads on any resources
> referenced before the html element.

How could anything be referenced before the HTML element? I can't think of any way except the Link HTTP header referencing a style sheet. How does Link header participate in the offline cache now?

The other thing I can think of is xml-stylesheet, but that's not relevant to the text/html parsing path.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> The speculative load requests are processed in the order the parser thread
> posted them, so if the cache manifest association was put onto that same side
> channel, it would happen before any scripts. Also, since there can be no
> scripts before the HTML element, the association wouldn't really be speculative
> but always a sure thing despite being on the speculative side channel for
> runnable ordering.

Understand.  I don't exactly know how the parser works, but what you suggest is to plan eTreeOpProcessOfflineManifest operation be done on the speculative event queue before any other loads (this applies to images or css as well!), right?

> Does the code actually want to see the HTML element with the manifest attribute
> in the DOM? 

AFAIKT, no.

> Would the code work if it got told that a given URL is the manifest
> URL before the HTML element exists?

As above, no.

> What would be the observable difference of running the cache selection
> algorithm when the document object exists but the root element doesn't compared
> to deferring it to the moment when the root element exists?

I'd say none.

> The <base> element can't participate in complicating the URL absolutization at
> either point. And xml:base isn't available in the HTML serialization, so it
> can't break things, either.

Agree.

> > You may freely do pre-loads on any resources
> > referenced before the html element.
> 
> How could anything be referenced before the HTML element? I can't think of any
> way except the Link HTTP header referencing a style sheet. How does Link header
> participate in the offline cache now?
> 
> The other thing I can think of is xml-stylesheet, but that's not relevant to
> the text/html parsing path.

Just a note, I have seen some notes about this in the spec. Style sheets or anything before the html element doesn't need to count with application cache association. There is something said about that in [1].

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-xml

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Use the speculative loader for processing cache manifest for stream parser

Most of this patch is just making the speculative loader wrap the executor instead of wrapping the document.

I'm treating the event delivery problem in the second test case as a duplicate of bug 539896.

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 424558 [details] [diff] [review]
Use the speculative loader for processing cache manifest for stream parser

>+    inline void Init(eHtml5TreeOperation aOpCode, const nsAString& aString) {
>+      NS_PRECONDITION(mOpCode == eTreeOpUninitialized,
>+        "Op code must be uninitialized when initializing.");
>+
>+      PRInt32 len = aString.Length();
>+      PRUnichar* str = new PRUnichar[len + 1];
>+      const PRUnichar* start = aString.BeginReading();
>+      for (PRInt32 i = 0; i < len; ++i) {
>+        str[i] = start[i];
>+      }
>+      str[len] = 0;
>+
>+      mOpCode = aOpCode;
>+      mOne.unicharPtr = str;
>+    }

You can just use ToNewUnicode(const nsAString&) to copy the string:
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#1416

Otherwise this patch looks great.

Comment 11

[Henri Sivonen (:hsivonen)]

Pushed http://hg.mozilla.org/mozilla-central/rev/18a259aece76
I used NS_StringCloneData since ToNewUnicode is deprecated. Filed bug 545844 as a follow-up about using NS_CStringCloneData, too.