1120649 - document.lastModified doesn't return the current time

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[James Graham [:jgraham]]

Per the HTML spec document.lastModified should return the current date/time if no other information is supplied with the document. We seem to instead return a single value that's either the initial load time of the document, or the time at which document.lastModified was first accessed, I'm not sure which.

See http://w3c-test.org/html/dom/documents/resource-metadata-management/document-lastModified-01.html

Comment 1

[Josh Matthews [:jdm]]

The relevant code here is nsDocument.cpp, in GetLastModified and RetrieveRelevantHeaders. In the latter, we set mLastModified to a value derived from PR_Now() if there's no better value present, so we always return that value in GetLastModified. Instead, we should leave mLastModified empty and return the string-formatted value of PR_Now() when GetLastModified is called.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think this is a spec bug.  It really doesn't make sense for lastModified to change during the lifetime of the document...

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27817

Comment 3

[Boris Zbarsky [:bzbarsky]]

I've given up on the spec here being sane.  We should just do whatever will placate the silly tests and not worry about this API being useful to anyone.

Comment 4

[Anirudh GP(:anirudhgp)]

Hi I'd like to work on this bug. Could you give me some pointers on how to fix this?

Comment 5

[Josh Matthews [:jdm]]

Are there parts of comment 1 that are unclear?

Comment 6

[Dhi Aurrahman]

Attachment: document-last-modified-should-return-current-time.patch

Comment 7

[Dhi Aurrahman]

Oops, I didn't see Anirudh is taking it. Sorry. (Well, my patch has possibility to be not the best way to fix this bug). Sorry again.

Comment 8

[Josh Matthews [:jdm]]

That's the solution I had in mind, but I think we should create a new function that takes a PRTime value and returns a string and call it from both places, rather than copying the code to format the time into a string.

Comment 9

[Dhi Aurrahman]

Attachment: document-last-modified-should-return-current-time.patch

Still unsure with the naming and its declaration (GetFormattedTimeString() is declared as member of nsIDocument and accept aFormattedTimeString by reference to sending out the formatted time string)

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8555230 [details] [diff] [review]
document-last-modified-should-return-current-time.patch

Review of attachment 8555230 [details] [diff] [review]:
-----------------------------------------------------------------

I won't be able to officially review this (I'm not on the list at https://wiki.mozilla.org/Modules/Core#Document_Object_Model), but this looks good to me :)

::: dom/base/nsDocument.cpp
@@ +3108,5 @@
> +  }
> +}
> +
> +void
> +nsIDocument::GetFormattedTimeString(const PRTime& aTime, nsAString& aFormattedTimeString) const

No need for this to be a member function; it can just be `static GetFormattedTimeString(PRTime aTime, nsAString& aFormattedTimeString)`.

Comment 11

[Dhi Aurrahman]

Attachment: document-last-modified-should-return-current-time.patch

Update as suggested by Josh.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8557031 [details] [diff] [review]
document-last-modified-should-return-current-time.patch

Looks good, thanks.  You generally want to add a commit message too, in the future.

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=434cde5f13b0

Comment 14

[Boris Zbarsky [:bzbarsky]]

This patch has an unexpected pass (leading to a test failure):

TEST-UNEXPECTED-PASS | /html/dom/documents/resource-metadata-management/document-lastModified-01.html | Date returned by lastModified is current after timeout. - expected FAIL 

Presumably you want to remove the corresponding annotation in testing/web-platform/meta/html/dom/documents/resource-metadata-management/document-lastModified-01.html.ini but it's not clear to me why the "Date returned by lastModified is current." is still failing...

Comment 15

[Dhi Aurrahman]

Hi Boris,

I'll try to check it out

Comment 16

[Dhi Aurrahman]

I'm not sure if it is related. Since I couldn't find a proper way to run the web-platform-test (./mach web-platform-test fails) I tried to run a $ python -m SimpleHTTPServer inside the testing/web-platform/tests directory. I found that the /resource/testharnessreport.js reports error all the time (window.opener is null) hence it cannot report back the test results to the main window. I also tried to copy-and-paste the test from w3c (all of related resources incuding the /resource/testharnessreport.js) got some errors because the document.lastModified is somehow static (I suspect it is because of the SimpleHTTPServer).

My question: is the /resource/testharnessreport.js should be the same as the w3c version or not? Well we can pass the http://w3c-test.org/html/dom/documents/resource-metadata-management/document-lastModified-01.html

And, I'm not sure what is the "remove the corresponding annotation" means. Does it mean skipping the test?

Comment 17

[Dhi Aurrahman]

./mach web-platform-tests

Comment 18

[Josh Matthews [:jdm]]

The current expected failures are annotated in http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/html/dom/documents/resource-metadata-management/document-lastModified-01.html.ini . When we remove them from that file, that means the test suite is expected to pass that test, and it is a failure if it does not.

Have you tried `./mach web-platform-tests --include=html/dom/documents/resource-metadata-management/document-lastModified-01.html`?

Comment 19

[James Graham [:jgraham]]

(In reply to Dhi Aurrahman from comment #16)
> I'm not sure if it is related. Since I couldn't find a proper way to run the
> web-platform-test (./mach web-platform-test fails)

What happens when you try to run it?

> I tried to run a $ python
> -m SimpleHTTPServer inside the testing/web-platform/tests directory. I found
> that the /resource/testharnessreport.js reports error all the time
> (window.opener is null) hence it cannot report back the test results to the
> main window.

Yes, that file is designed as the integration point with the test harness and won't work in other scenarios. It is intentionally different from upstream.

> I also tried to copy-and-paste the test from w3c (all of
> related resources incuding the /resource/testharnessreport.js) got some
> errors because the document.lastModified is somehow static (I suspect it is
> because of the SimpleHTTPServer).
> 
> My question: is the /resource/testharnessreport.js should be the same as the
> w3c version or not? Well we can pass the
> http://w3c-test.org/html/dom/documents/resource-metadata-management/document-
> lastModified-01.html

It seems pretty surprising if you can get the w3c version of the test to pass but the local one still fails. Can you verify that the two tests are the same? 

> And, I'm not sure what is the "remove the corresponding annotation" means.
> Does it mean skipping the test?

Hopefully jdm's explanation makes sense here.

Comment 20

[Dhi Aurrahman]

Attachment: bug-1120649-fix.patch remove related web-platform-tests annotations

Removed the web-platform-tests annotations testing/web-platform/meta/html/dom/documents/resource-metadata-management/document-lastModified-01.html.ini

Comment 21

[Dhi Aurrahman]

(In reply to James Graham [:jgraham] from comment #19)
> (In reply to Dhi Aurrahman from comment #16)
> > I'm not sure if it is related. Since I couldn't find a proper way to run the
> > web-platform-test (./mach web-platform-test fails)
> 
> What happens when you try to run it?

After reboot it is fine, but only runs once. The second attempt running it, the test refuses to run.

```
The details of the failure are as follows:

EnvironmentError: http server on port 8000 failed to start

  File "/Volumes/Backup/workspace/mozilla/bundles/mozilla-central/testing/web-platform/mach_commands.py", line 132, in run_web_platform_tests
    return wpt_runner.run_tests(**params)
  File "/Volumes/Backup/workspace/mozilla/bundles/mozilla-central/testing/web-platform/mach_commands.py", line 73, in run_tests
    result = wptrunner.run_tests(**kwargs)
  File "/Volumes/Backup/workspace/mozilla/bundles/mozilla-central/testing/web-platform/harness/wptrunner/wptrunner.py", line 415, in run_tests
    test_environment.ensure_started()
  File "/Volumes/Backup/workspace/mozilla/bundles/mozilla-central/testing/web-platform/harness/wptrunner/wptrunner.py", line 253, in ensure_started
    "%s server on port %d failed to start" % (scheme, port))
```
> 
> > I tried to run a $ python
> > -m SimpleHTTPServer inside the testing/web-platform/tests directory. I found
> > that the /resource/testharnessreport.js reports error all the time
> > (window.opener is null) hence it cannot report back the test results to the
> > main window.
> 
> Yes, that file is designed as the integration point with the test harness
> and won't work in other scenarios. It is intentionally different from
> upstream.

Thanks, my bad, I've just read the comments on that file.

> 
> > I also tried to copy-and-paste the test from w3c (all of
> > related resources incuding the /resource/testharnessreport.js) got some
> > errors because the document.lastModified is somehow static (I suspect it is
> > because of the SimpleHTTPServer).
> > 
> > My question: is the /resource/testharnessreport.js should be the same as the
> > w3c version or not? Well we can pass the
> > http://w3c-test.org/html/dom/documents/resource-metadata-management/document-
> > lastModified-01.html
> 
> It seems pretty surprising if you can get the w3c version of the test to
> pass but the local one still fails. Can you verify that the two tests are
> the same? 

Yes the two tests are the same. As I said, after reboot I can run the web-platform-tests without problem. I guess this is only my env problem.

> 
> > And, I'm not sure what is the "remove the corresponding annotation" means.
> > Does it mean skipping the test?
> 
> Hopefully jdm's explanation makes sense here.

Yes. Thank you!

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8559084 [details] [diff] [review]
bug-1120649-fix.patch remove related web-platform-tests annotations

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97680662ba41

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1660c10f980b

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1660c10f980b

Comment 26

[Manish Goregaokar [:manishearth]]

We got another orange on this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1064625#c67

Probably due to daylight savings? I guess we should ensure that the timezone doesn't change here, and then unmark the test as an orange.