document.lastModified doesn't return the current time

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jgraham, Assigned: diorahman, Mentored)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
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.
Mentor: josh
Whiteboard: [lang=c++]
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

Updated

4 years ago
Mentor: josh
Whiteboard: [lang=c++]
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.

Updated

4 years ago
Mentor: josh
Whiteboard: [lang=c++]
Hi I'd like to work on this bug. Could you give me some pointers on how to fix this?

Comment 5

4 years ago
Are there parts of comment 1 that are unclear?
(Assignee)

Comment 6

4 years ago
Created attachment 8555010 [details] [diff] [review]
document-last-modified-should-return-current-time.patch
(Assignee)

Comment 7

4 years ago
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.

Updated

4 years ago
Assignee: nobody → diorahman

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8555230 [details] [diff] [review]
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)
Attachment #8555010 - Attachment is obsolete: true
Attachment #8555230 - Flags: review?(josh)
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)`.
Attachment #8555230 - Flags: review?(josh) → feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 8557031 [details] [diff] [review]
document-last-modified-should-return-current-time.patch

Update as suggested by Josh.
Attachment #8555230 - Attachment is obsolete: true
Attachment #8557031 - Flags: review?(hsivonen)
(Assignee)

Updated

4 years ago
Attachment #8557031 - Flags: review?(hsivonen) → review?(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.
Attachment #8557031 - Flags: review?(bzbarsky) → review+
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...
Flags: needinfo?(diorahman)
Keywords: checkin-needed
(Assignee)

Comment 15

4 years ago
Hi Boris,

I'll try to check it out
Flags: needinfo?(diorahman)
(Assignee)

Comment 16

4 years ago
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?
Flags: needinfo?(josh)
(Assignee)

Comment 17

4 years ago
./mach web-platform-tests
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`?
Flags: needinfo?(josh)
(Reporter)

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
Created attachment 8559084 [details] [diff] [review]
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
Attachment #8557031 - Attachment is obsolete: true
Attachment #8559084 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

4 years ago
(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 on attachment 8559084 [details] [diff] [review]
bug-1120649-fix.patch remove related web-platform-tests annotations

r=me
Attachment #8559084 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1660c10f980b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
You need to log in before you can comment on or make changes to this bug.