Preloads should be cleared when JavaScript cancels loading prematurely.

[James Simonsen]

Preloads should be cleared on document.write()

[James Simonsen]

Created attachment 189386 [details]
Patch

[James Simonsen]

This was discovered and reported internally at Google. Preloaded resources were being reused after a document.write() even though they were explicitly marked no-cache.

[Adam Barth]

Comment on attachment 189386 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4407
> +    // Parser should have picked up all preloads by now
> +    m_cachedResourceLoader->clearPreloads();

This isn't true for the threaded parser.  The threaded parser doesn't finish synchronously.

[James Simonsen]

(In reply to comment #3)
> (From update of attachment 189386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189386&action=review
> 
> > Source/WebCore/dom/Document.cpp:4407
> > +    // Parser should have picked up all preloads by now
> > +    m_cachedResourceLoader->clearPreloads();
> 
> This isn't true for the threaded parser.  The threaded parser doesn't finish synchronously.

It calls finishedParsing() before it's actually finished?

[WebKit Review Bot]

Comment on attachment 189386 [details]
Patch

Attachment 189386 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16660547

New failing tests:
fast/js/regress/inlined-put-by-id-transition.html

[James Simonsen]

(In reply to comment #5)
> (From update of attachment 189386 [details])
> Attachment 189386 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16660547
> 
> New failing tests:
> fast/js/regress/inlined-put-by-id-transition.html

This appears to be a flake. It doesn't reproduce locally and seems totally unrelated.

[Antti Koivisto]

Comment on attachment 189386 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +
> +        We should clear preloads as soon as the parser finishes, instead of waiting for
> +        implicitClose(), which doesn't happen with a document.write().

Why? Please describe the problem you are fixing in the ChangeLog.

[Antti Koivisto]

(In reply to comment #2)
> This was discovered and reported internally at Google. Preloaded resources were being reused after a document.write() even though they were explicitly marked no-cache.

What makes you think this is a bug?

[James Simonsen]

Created attachment 189421 [details]
Patch

[James Simonsen]

(In reply to comment #7)
> (From update of attachment 189386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189386&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +
> > +        We should clear preloads as soon as the parser finishes, instead of waiting for
> > +        implicitClose(), which doesn't happen with a document.write().
> 
> Why? Please describe the problem you are fixing in the ChangeLog.

Done.

[Adam Barth]

> It calls finishedParsing() before it's actually finished?

Nope.  You're right!  I was confused with finishParsing.  :)

[Antti Koivisto]

Comment on attachment 189421 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Preloads should be cleared on document.write()
> +        https://bugs.webkit.org/show_bug.cgi?id=110388
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        If a page preloads a resource, then uses document.write(), that resource is forever marked
> +        as a preload and will always be fetched from cache even after an explicit reload. Instead,
> +        we should clear the preloads as soon as the parser finishes, rather than waiting for
> +        implicitClose.

I still don't quite understand. What does document.write have to do with this? Your test case does not use document.write.

[James Simonsen]

Created attachment 189568 [details]
Patch

[James Simonsen]

(In reply to comment #12)
> (From update of attachment 189421 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189421&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Preloads should be cleared on document.write()
> > +        https://bugs.webkit.org/show_bug.cgi?id=110388
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        If a page preloads a resource, then uses document.write(), that resource is forever marked
> > +        as a preload and will always be fetched from cache even after an explicit reload. Instead,
> > +        we should clear the preloads as soon as the parser finishes, rather than waiting for
> > +        implicitClose.
> 
> I still don't quite understand. What does document.write have to do with this? Your test case does not use document.write.

Oh. Good point. I must've inadvertently removed that while I was working on it.  It seems both cases cause problems though, so I've updated the description to match.

Basically (if it's not clear already), the problem is that some resources get permanently marked as preloads. When you reload the page, we reuse the resource from memory cache because it's still marked as as preload even though the resource is explicitly no-cache. So, we need to more aggressively clear out the preloads when JavaScript stops the parser.

[James Simonsen]

Created attachment 189569 [details]
Patch

[Adam Barth]

Antti, did you have further comments on this patch?

[Antti Koivisto]

Comment on attachment 189569 [details]
Patch

Ok, r=me

[WebKit Review Bot]

Comment on attachment 189569 [details]
Patch

Clearing flags on attachment: 189569

Committed r143789: <http://trac.webkit.org/changeset/143789>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.