Bug 131318 - Web Replay: memoize fallback time values for document.lastModified
Summary: Web Replay: memoize fallback time values for document.lastModified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Burg
URL:
Keywords:
Depends on: 129391 130728 131324 131667
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-07 15:42 PDT by Brian Burg
Modified: 2014-04-16 17:57 PDT (History)
7 users (show)

See Also:


Attachments
the patch (49.61 KB, patch)
2014-04-07 16:56 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
the patch (49.61 KB, patch)
2014-04-07 17:01 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
OK, this is actually it (5.38 KB, patch)
2014-04-07 17:02 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
patch with updated TestExpectations (7.23 KB, patch)
2014-04-15 17:24 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-04-07 15:42:58 PDT
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.
Comment 1 Brian Burg 2014-04-07 16:56:38 PDT
Created attachment 228772 [details]
the patch
Comment 2 Brian Burg 2014-04-07 16:57:03 PDT
Comment on attachment 228772 [details]
the patch

wrong patch, i hate you webkit-patch
Comment 3 Brian Burg 2014-04-07 17:01:16 PDT
(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
Comment 4 Brian Burg 2014-04-07 17:01:55 PDT
Created attachment 228774 [details]
the patch
Comment 5 Brian Burg 2014-04-07 17:02:54 PDT
Created attachment 228775 [details]
OK, this is actually it
Comment 6 Brian Burg 2014-04-07 17:06:26 PDT
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.
Comment 7 Brian Burg 2014-04-07 19:08:59 PDT
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.
Comment 8 Joseph Pecoraro 2014-04-07 19:14:22 PDT
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?
Comment 9 WebKit Commit Bot 2014-04-14 12:45:51 PDT
Comment on attachment 228775 [details]
OK, this is actually it

Clearing flags on attachment: 228775

Committed r167261: <http://trac.webkit.org/changeset/167261>
Comment 10 WebKit Commit Bot 2014-04-14 12:45:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Alexey Proskuryakov 2014-04-14 23:20:47 PDT
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.
Comment 12 WebKit Commit Bot 2014-04-14 23:21:52 PDT
Re-opened since this is blocked by bug 131667
Comment 13 Alexey Proskuryakov 2014-04-14 23:22:08 PDT
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.
Comment 14 Brian Burg 2014-04-14 23:31:03 PDT
(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.
Comment 15 Brian Burg 2014-04-15 16:40:14 PDT
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.
Comment 16 Alexey Proskuryakov 2014-04-15 16:55:55 PDT
> So, can this be re-landed with updated test expectations?

Seems worth trying.
Comment 17 Brian Burg 2014-04-15 17:24:31 PDT
Created attachment 229419 [details]
patch with updated TestExpectations
Comment 18 WebKit Commit Bot 2014-04-16 17:57:05 PDT
Comment on attachment 229419 [details]
patch with updated TestExpectations

Clearing flags on attachment: 229419

Committed r167406: <http://trac.webkit.org/changeset/167406>
Comment 19 WebKit Commit Bot 2014-04-16 17:57:10 PDT
All reviewed patches have been landed.  Closing bug.