Bug 47035 – Application cache selection algorithm should only be invoked after navigation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

We don't implement this part of HTML5:

------------------
A start tag whose tag name is "html"
Create an element for the token in the HTML namespace. Append it to the Document object. Put this element in the stack of open elements.

If the Document is being loaded as part of navigation of a browsing context, then: if the newly created element has a manifest attribute whose value is not the empty string, then resolve the value of that attribute to an absolute URL, relative to the newly created element, and if that is successful, run the application cache selection algorithm with the resulting absolute URL with any <fragment> component removed; otherwise, if there is no such attribute, or its value is the empty string, or resolving its value fails, run the application cache selection algorithm with no manifest. The algorithm must be passed the Document object.
------------------

Instead, we always run the application cache selection algorithm, even if not loaded as part of navigation of a browsing context. That causes assertion failures and generally incorrect behavior.

[Alexey Proskuryakov]

Created attachment 69551 [details]
test case (http/tests/appcache)

Firefox 3.6.10 has really weird behavior on this test - it passes at first, but fails when reloading the page.

[Alexey Proskuryakov]

An assertion fails if you e.g. just document.write("foobar") in a document associated with an appcache.

[Alexey Proskuryakov]

Created attachment 69565 [details]
proposed fix

[Adam Barth]

Comment on attachment 69565 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=69565&action=review

Please add a test case for an HTMLHtmlElement created by createElement being added to the document while the parser is running.

> WebCore/dom/DocumentParser.h:92
> +    void setWasLoadedAsPartOfNavigation() { m_wasLoadedAsPartOfNavigation = true; }
> +    bool wasLoadedAsPartOfNavigation() const { return m_wasLoadedAsPartOfNavigation; }

Can we make this an argument to the constructor?  This API is much wider than we need.  Also, the getter can be protected if we the parser itself make the check.  Adding it to the constructor will also let us scope the bit to the HTMLDocumentParser, where it seems to belong according to the spec.

> WebCore/html/HTMLHtmlElement.cpp:65
> -    if (!document()->parsing())
> +    if (!document()->parser() || !document()->parser()->wasLoadedAsPartOfNavigation())
>          return;

This code looks wrong.  It only supposed to trigger for HTMLHtmlElement created by the parser.  It's entirely possible to createElement('html') and insert it into the document while the document has a parser that was loaded as part of navigation.

Instead of putting this code in insertedIntoDocument, we should put the code in a new method of HTMLHtmlElement that the parser calls explicitly in the proper state.

> WebCore/loader/FrameLoader.cpp:614
> +    m_frame->document()->parser()->setWasLoadedAsPartOfNavigation();

This works should be done by the DocumentWriter.  It's responsible for interacting with the parser.

[Alexey Proskuryakov]

> Please add a test case for an HTMLHtmlElement created by createElement being added to the document while the parser is running.

Will do.

> Can we make this an argument to the constructor?  This API is much wider than we need.  Also, the getter can be protected if we the parser itself make the check.  Adding it to the constructor will also let us scope the bit to the HTMLDocumentParser, where it seems to belong according to the spec.

XHTML is not different at all. Image documents etc also need this function to run to completion, for "select cache without manifest URL" branch to be taken. See section 6.5.3 "Page load processing model for XML files" and below for non-HTML content.

In fact, this is why I made it a separate function, not a constructor argument.

> This code looks wrong.  It only supposed to trigger for HTMLHtmlElement created by the parser.  It's entirely possible to createElement('html') and insert it into the document while the document has a parser that was loaded as part of navigation.
> 
> Instead of putting this code in insertedIntoDocument, we should put the code in a new method of HTMLHtmlElement that the parser calls explicitly in the proper state.

Well, that would mean moving all of insertedIntoDocument() into this special function, for every document kind... Appcache cache selection is the only reason for its existence.

Perhaps that's what has to be be done indeed.

> > WebCore/loader/FrameLoader.cpp:614
> > +    m_frame->document()->parser()->setWasLoadedAsPartOfNavigation();
> 
> This works should be done by the DocumentWriter.  It's responsible for interacting with the parser.

Why is the parser exposed on Document if we can't talk to it? Sure, I could pass the arguments all the way through DocumentWriter constructor, and all Document derived class constructors, but I doubt that we gain more than we lose that way.

[Adam Barth]

> > This code looks wrong.  It only supposed to trigger for HTMLHtmlElement created by the parser.  It's entirely possible to createElement('html') and insert it into the document while the document has a parser that was loaded as part of navigation.
> > 
> > Instead of putting this code in insertedIntoDocument, we should put the code in a new method of HTMLHtmlElement that the parser calls explicitly in the proper state.
> 
> Well, that would mean moving all of insertedIntoDocument() into this special function, for every document kind... Appcache cache selection is the only reason for its existence.
> 
> Perhaps that's what has to be be done indeed.

Is there another way to get the right behavior?  Whether to trigger this code depends on who's inserting the element into the document.

> > > WebCore/loader/FrameLoader.cpp:614
> > > +    m_frame->document()->parser()->setWasLoadedAsPartOfNavigation();
> > 
> > This works should be done by the DocumentWriter.  It's responsible for interacting with the parser.
> 
> Why is the parser exposed on Document if we can't talk to it? Sure, I could pass the arguments all the way through DocumentWriter constructor, and all Document derived class constructors, but I doubt that we gain more than we lose that way.

The abstraction boundaries in FrameLoader are very poor.  I have a long-term plan for cleaning them up, but that first requires moving all the code to the right place and then limiting the visibility of things you're not supposed to see.

This code shouldn't go in FrameLoader because FrameLoader shouldn't interact with the parser at all.  If you look at this diagram:

https://docs1.google.com/drawings/edit?id=1ko0LFteYpoXdmfYO1rYme6t-QXLPQdI1Z_ysejpOVYk&hl=en

You'll see that it's DocumentLoader's job to interact with the parser.  FrameLoader's job is to shuffle DocumentLoaders around for the provisional/committed lifecycle.  DocumentLoader's job is to actually shove bytes into the document.  I've managed to package most of FrameLoader's interaction with the parser up into DocumentWriter.  The next step is to move DocumentWrite from FrameLoader to DocumentLoader (and probably give it a better name).

I know all this stuff isn't super visible in the code at the moment, but hopefully it's on the road to improvement.

[Alexey Proskuryakov]

Created attachment 69658 [details]
proposed fix

[Early Warning System Bot]

Attachment 69658 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4252002

[Alexey Proskuryakov]

Created attachment 69663 [details]
with Qt build fix

[Adam Barth]

Comment on attachment 69663 [details]
with Qt build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=69663&action=review

I'm not super happy with how the bit ends up getting set on the parser, but I'm not sure I have a better idea for how to do it.  Thanks for iterating on the patch.

> WebCore/html/ImageDocument.cpp:200
>      RefPtr<Element> rootElement = Document::createElement(htmlTag, false);
>      appendChild(rootElement, ec);
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> +    static_cast<HTMLHtmlElement*>(rootElement.get())->insertedByParser();
> +#endif

I should clean up all this stuff to go through the HTMLDocumentParser.

> WebCore/html/parser/HTMLConstructionSite.h:-57
> -    void insertHTMLHtmlElement(AtomicHTMLToken&);

I take it this was unused.

> WebCore/loader/DocumentWriter.cpp:254
> +void DocumentWriter::setDocumentWasLoadedAsPartOfNavigation()
> +{
> +    m_frame->document()->parser()->setDocumentWasLoadedAsPartOfNavigation();
> +}

This does kind of look silly.  :)

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/69026>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69026 might have broken Qt Windows 32-bit Release

[Alexey Proskuryakov]

For the record, loaded as part of navigations means being loaded by the "Navigating across documents" algorithm: <http://www.whatwg.org/specs/web-apps/current-work/complete/history.html#navigate>