Bug 143463 – Bing video search result pages are not PageCacheable

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Bing video search result pages are not PageCacheable (tested on iOS). It both:
- is bad for power usage as it causes a reload when clicking one of the results then navigating back in history
- degrades user experience because the results page uses infinite scrolling and the scroll position is not properly restored when navigating back, not to mention the user has to wait for the reload to complete.

Radar: <rdar://problem/20440916>

[Chris Dumez]

Created attachment 250252 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 250252 [details]
Patch

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

> LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition.html:11
> +    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);

Is this preference reset after the test (in both DRT and WKTR)?

[Alexey Proskuryakov]

Please fix the iOS build:

/Volumes/Data/EWS/WebKit/Source/WebKit/mac/Misc/WebCache.mm:204:81: error: no member named 'resourceForURL' in 'WebCore::MemoryCache'
    WebCore::CachedResource* cachedResource = WebCore::MemoryCache::singleton().resourceForURL(url);

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 250252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250252&action=review
> 
> > LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition.html:11
> > +    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
> 
> Is this preference reset after the test (in both DRT and WKTR)?

Yes, for WKTR, this is taken care of by WebPreferencesStore::removeTestRunnerOverrides().
For DTR, in void resetWebPreferencesToConsistentValues() in DumpRenderTree.mm:
[preferences setUsesPageCache:NO];

[Chris Dumez]

Created attachment 250254 [details]
Patch

[Chris Dumez]

Created attachment 250255 [details]
Patch

[Chris Dumez]

Created attachment 250256 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 250256 [details]
Patch

Clearing flags on attachment: 250256

Committed r182449: <http://trac.webkit.org/changeset/182449>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Looks like this broke a test on Windows:

http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml

@@ -1,2 +1,6 @@
+CONSOLE MESSAGE: line 1: Document is empty
+
+CONSOLE MESSAGE: line 1: Start tag expected, '<' not found
+
 This test includes a cross-origin external entity. It passes if the load fails and thus there is no text below this line.

[Chris Dumez]

(In reply to comment #10)
> Looks like this broke a test on Windows:
> 
> http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml
> 
> @@ -1,2 +1,6 @@
> +CONSOLE MESSAGE: line 1: Document is empty
> +
> +CONSOLE MESSAGE: line 1: Start tag expected, '<' not found
> +
>  This test includes a cross-origin external entity. It passes if the load
> fails and thus there is no text below this line.

Hmm, this is really odd. I'll take a look now.

[Chris Dumez]

The failing test does not use PageCache or internals.isLoadingFromMemoryCache(). Could it be a flake?

[Alexey Proskuryakov]

It fails every time, so not really a flake:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2Fxss-DENIED-xsl-external-entity-redirect.xml

[Chris Dumez]

(In reply to comment #13)
> It fails every time, so not really a flake:
> 
> http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.
> html#tests=http%2Ftests%2Fsecurity%2Fxss-DENIED-xsl-external-entity-redirect.
> xml

Ok, still investigating. stdio shows:
476 2928 worker/0 http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml passed
18:00:06.538 2928 worker/0 http/tests/security/xss-DENIED-xsl-external-entity.xml output stderr lines:
18:00:06.538 2928   ERROR: Failed to create path /tmp/DumpRenderTree-1sDSaK\Databases
18:00:06.538 2928   ..\platform\win\FileSystemWin.cpp(212) : WebCore::makeAllDirectories
18:00:06.538 2928   ERROR: Failed to create path /tmp/DumpRenderTree-1sDSaK\Databases
18:00:06.538 2928   ..\platform\win\FileSystemWin.cpp(212) : WebCore::makeAllDirectories
18:00:06.538 2928   LEAK: 3 CachedPage
18:00:06.538 2928   LEAK: 218 CachedResource
18:00:06.538 2928   LEAK: 4 CachedFrame
18:00:06.538 2928   LEAK: 7 Frame
18:00:06.538 2928   LEAK: 242 WebCoreNode
18:00:06.538 2928   LEAK: 46 RenderObject
18:00:06.538 2928   LEAK: 6 Page
18:00:06.538 2928   file:///C:/Program%20Files%20(x86)/Common%20Files/Apple/Apple%20Application%20Support/../etc/catalog:1: parser error : Document is empty
18:00:06.538 2928   
18:00:06.538 2928   ^
18:00:06.538 2928   file:///C:/Program%20Files%20(x86)/Common%20Files/Apple/Apple%20Application%20Support/../etc/catalog:1: parser error : Start tag expected, '<' not found
18:00:06.538 2928   
18:00:06.538 2928   ^
18:00:06.538 2928   compilation error: file http://127.0.0.1:8000/security/xss-DENIED-xsl-document-securityOrigin.xml line 2 element stylesheet
18:00:06.538 2928   xsl:version: only 1.0 features are supported
18:00:06.538 2928   compilation error: file http://127.0.0.1:8000/security/xss-DENIED-xsl-document-securityOrigin.xml line 2 element stylesheet
18:00:06.538 2928   xsl:version: only 1.0 features are supported
18:00:06.538 2928   compilation error: file http://127.0.0.1:8000/security/xss-DENIED-xsl-document-securityOrigin.xml line 2 element stylesheet
18:00:06.538 2928   xsl:version: only 1.0 features are supported
18:00:06.538 2928 worker/0 http/tests/security/xss-DENIED-xsl-external-entity.xml passed

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > It fails every time, so not really a flake:
> > 
> > http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.
> > html#tests=http%2Ftests%2Fsecurity%2Fxss-DENIED-xsl-external-entity-redirect.
> > xml
> 
> Ok, still investigating. stdio shows:
> 476 2928 worker/0
> http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml passed
> 18:00:06.538 2928 worker/0
> http/tests/security/xss-DENIED-xsl-external-entity.xml output stderr lines:
> 18:00:06.538 2928   ERROR: Failed to create path
> /tmp/DumpRenderTree-1sDSaK\Databases
> 18:00:06.538 2928   ..\platform\win\FileSystemWin.cpp(212) :
> WebCore::makeAllDirectories
> 18:00:06.538 2928   ERROR: Failed to create path
> /tmp/DumpRenderTree-1sDSaK\Databases
> 18:00:06.538 2928   ..\platform\win\FileSystemWin.cpp(212) :
> WebCore::makeAllDirectories
> 18:00:06.538 2928   LEAK: 3 CachedPage
> 18:00:06.538 2928   LEAK: 218 CachedResource
> 18:00:06.538 2928   LEAK: 4 CachedFrame
> 18:00:06.538 2928   LEAK: 7 Frame
> 18:00:06.538 2928   LEAK: 242 WebCoreNode
> 18:00:06.538 2928   LEAK: 46 RenderObject
> 18:00:06.538 2928   LEAK: 6 Page
> 18:00:06.538 2928  
> file:///C:/Program%20Files%20(x86)/Common%20Files/Apple/
> Apple%20Application%20Support/../etc/catalog:1: parser error : Document is
> empty
> 18:00:06.538 2928   
> 18:00:06.538 2928   ^
> 18:00:06.538 2928  
> file:///C:/Program%20Files%20(x86)/Common%20Files/Apple/
> Apple%20Application%20Support/../etc/catalog:1: parser error : Start tag
> expected, '<' not found
> 18:00:06.538 2928   
> 18:00:06.538 2928   ^
> 18:00:06.538 2928   compilation error: file
> http://127.0.0.1:8000/security/xss-DENIED-xsl-document-securityOrigin.xml
> line 2 element stylesheet
> 18:00:06.538 2928   xsl:version: only 1.0 features are supported
> 18:00:06.538 2928   compilation error: file
> http://127.0.0.1:8000/security/xss-DENIED-xsl-document-securityOrigin.xml
> line 2 element stylesheet
> 18:00:06.538 2928   xsl:version: only 1.0 features are supported
> 18:00:06.538 2928   compilation error: file
> http://127.0.0.1:8000/security/xss-DENIED-xsl-document-securityOrigin.xml
> line 2 element stylesheet
> 18:00:06.538 2928   xsl:version: only 1.0 features are supported
> 18:00:06.538 2928 worker/0
> http/tests/security/xss-DENIED-xsl-external-entity.xml passed

Interestingly, stdio shows the same stdio errors in the builds before mine:
https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/65579/steps/layout-test/logs/stdio

However, the test was not recognized as failing before (same errors though).

[Chris Dumez]

Looks like the Windows failure is caused by <rdar://problem/4292995>. mac and iOS already have a workaround for it, we likely want to do the same for win?
Tools/Scripts/webkitpy/port/ios.py:        env['XML_CATALOG_FILES'] = ''  # work around missing /etc/catalog <rdar://problem/4292995>
Tools/Scripts/webkitpy/port/mac.py:        env['XML_CATALOG_FILES'] = ''  # work around missing /etc/catalog <rdar://problem/4292995>

[Brent Fulgham]

(In reply to comment #16)
> Looks like the Windows failure is caused by <rdar://problem/4292995>. mac
> and iOS already have a workaround for it, we likely want to do the same for
> win?
> Tools/Scripts/webkitpy/port/ios.py:        env['XML_CATALOG_FILES'] = ''  #
> work around missing /etc/catalog <rdar://problem/4292995>
> Tools/Scripts/webkitpy/port/mac.py:        env['XML_CATALOG_FILES'] = ''  #
> work around missing /etc/catalog <rdar://problem/4292995>

Wow -- we should definitely do the same for Windows!

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #16)
> > Looks like the Windows failure is caused by <rdar://problem/4292995>. mac
> > and iOS already have a workaround for it, we likely want to do the same for
> > win?
> > Tools/Scripts/webkitpy/port/ios.py:        env['XML_CATALOG_FILES'] = ''  #
> > work around missing /etc/catalog <rdar://problem/4292995>
> > Tools/Scripts/webkitpy/port/mac.py:        env['XML_CATALOG_FILES'] = ''  #
> > work around missing /etc/catalog <rdar://problem/4292995>
> 
> Wow -- we should definitely do the same for Windows!

Filed https://bugs.webkit.org/show_bug.cgi?id=143484.

[Alexey Proskuryakov]

Good catch!

Not sure if we have an explanation of why this started being a test failure, seems quite surprising.

[Chris Dumez]

(In reply to comment #19)
> Good catch!
> 
> Not sure if we have an explanation of why this started being a test failure,
> seems quite surprising.

Yes, somehow the stderr output ended up as CONSOLE messages, causing the layout tests to fail. Not sure how this can happen. It may even be the stderr output from the previous test if I read correctly...

[Alexey Proskuryakov]

These may be coming from XSLTProcessor::parseErrorFunc, so they are proper console messages from the beginning, not stderr messages.

[Alexey Proskuryakov]

The test is still failing with that fixed.

[Alexey Proskuryakov]

The test is still failing on Windows Debug.

[Chris Dumez]

(In reply to comment #23)
> The test is still failing on Windows Debug.

I believe Brent was looking into the XML error. However, I have another speculative fix at https://bugs.webkit.org/show_bug.cgi?id=143779.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > The test is still failing on Windows Debug.
> 
> I believe Brent was looking into the XML error. However, I have another
> speculative fix at https://bugs.webkit.org/show_bug.cgi?id=143779.

Looks like the test is no longer failing starting with the following build:
https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/65705

I don't know what fixed it but the test still passes in the builds that follow.

[Alexey Proskuryakov]

It's failing again.

[Chris Dumez]

(In reply to comment #26)
> It's failing again.

I don't know what Brent thinks about this but I would propose marking it was flaky. I don't believe the failure was caused by my change. The test clearly is flaky as it starts passing / failing for no apparent reason.