Bug 131318 – Web Replay: memoize fallback time values for document.lastModified

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brian Burg]

If we can't understand or find the Last-Modified header, then document.lastModified is derived from currentTimeMS(), which is obviously nondeterministic.

It's better to handle this inside Document::lastModified rather than using MemoizedDOMResult, because only the fallback case is nondeterministic.

[Brian Burg]

Created attachment 228772 [details]
the patch

[Brian Burg]

Comment on attachment 228772 [details]
the patch

wrong patch, i hate you webkit-patch

[Brian Burg]

(In reply to comment #2)
> (From update of attachment 228772 [details])
> wrong patch, i hate you webkit-patch

Reported: https://bugs.webkit.org/show_bug.cgi?id=131327

[Brian Burg]

Created attachment 228774 [details]
the patch

[Brian Burg]

Created attachment 228775 [details]
OK, this is actually it

[Brian Burg]

Comment on attachment 228775 [details]
OK, this is actually it

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

> Source/WebCore/ChangeLog:15
> +        Test: http/tests/inspector/replay/document-last-modified-fallback-value.html

This test will not actually work until 130728 and 129391 are fixed. Until then, the resource could have cached values of Last-Modified and be flaky. I didn't mark anything skip because LayoutTests/inspector/ is completely skipped due to 129642.

[Brian Burg]

BTW: this API is being addressed now, because it is used by the google analytics urchin. So, basically every page hits it once, then makes a network request with a URL based on it.

[Joseph Pecoraro]

Comment on attachment 228775 [details]
OK, this is actually it

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

r=me

> LayoutTests/http/tests/inspector/replay/document-last-modified-fallback-value.html:4
> +<script type="text/javascript" src="./replay-test.js"></script>

Is the "./" really needed here?

[WebKit Commit Bot]

Comment on attachment 228775 [details]
OK, this is actually it

Clearing flags on attachment: 228775

Committed r167261: <http://trac.webkit.org/changeset/167261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This test was landed without an expected result. But more importantly, it appears that this change broke a lot of tests:

http/tests/loading/unfinished-main-resource-back-to-cached-page-callbacks.html
http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
http/tests/misc/isindex-with-no-form.html
http/tests/navigation/javascriptlink-frames.html
http/tests/navigation/document-location-click.html
http/tests/navigation/javascriptlink-goback.html
http/tests/navigation/javascriptlink-basic.html
http/tests/navigation/document-location-click-timeout.html
http/tests/navigation/document-location-mouseover.html

Please see <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r167261%20(17688)/results.html> for results.

Will roll out. Unfortunately, bots were not being looked at today, and thus didn't work well, so I have to go by data from a single tester. Apologies in advance if this was not the culprit.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131667

[Alexey Proskuryakov]

Comment on attachment 228775 [details]
OK, this is actually it

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

>> Source/WebCore/ChangeLog:15
>> +        Test: http/tests/inspector/replay/document-last-modified-fallback-value.html
> 
> This test will not actually work until 130728 and 129391 are fixed. Until then, the resource could have cached values of Last-Modified and be flaky. I didn't mark anything skip because LayoutTests/inspector/ is completely skipped due to 129642.

Looks like this is not the case, because bots were running this test.

[Brian Burg]

(In reply to comment #11)
> This test was landed without an expected result. But more importantly, it appears that this change broke a lot of tests:
> 
> http/tests/loading/unfinished-main-resource-back-to-cached-page-callbacks.html
> http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
> http/tests/misc/isindex-with-no-form.html
> http/tests/navigation/javascriptlink-frames.html
> http/tests/navigation/document-location-click.html
> http/tests/navigation/javascriptlink-goback.html
> http/tests/navigation/javascriptlink-basic.html
> http/tests/navigation/document-location-click-timeout.html
> http/tests/navigation/document-location-mouseover.html
> 
> Please see <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r167261%20(17688)/results.html> for results.
> 
> Will roll out. Unfortunately, bots were not being looked at today, and thus didn't work well, so I have to go by data from a single tester. Apologies in advance if this was not the culprit.

Ugh, my IRC client apparently stopped working after dinner or I would have looked into it. I'll look at this tomorrow on a ML debug build.

[Brian Burg]

After some fiddling and test runs, my hypothesis at the moment is that the new test was running even though it shouldn't have. If it encountered an error and failed to clean up/restore non-replay settings (say, leaving page cache turned off), this may mess up later tests on the same process. 

(It's disturbing to me that I cannot confidently say that the test harness  resets obvious settings like that.)


When I explicitly skip the test in TestExpectations, I can no longer reproduce the problem on my WK2-ML-debug build.

So, can this be re-landed with updated test expectations? I don't think it's a productive use of time to try and enable this test right now, since it clearly depends on other replay support that hasn't landed yet.

[Alexey Proskuryakov]

> So, can this be re-landed with updated test expectations?

Seems worth trying.

[Brian Burg]

Created attachment 229419 [details]
patch with updated TestExpectations

[WebKit Commit Bot]

Comment on attachment 229419 [details]
patch with updated TestExpectations

Clearing flags on attachment: 229419

Committed r167406: <http://trac.webkit.org/changeset/167406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.