Closed
Bug 1120649
Opened 9 years ago
Closed 9 years ago
document.lastModified doesn't return the current time
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: jgraham, Assigned: diorahman, Mentored)
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 3 obsolete files)
3.91 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•9 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++]
Comment 2•9 years ago
|
||
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•9 years ago
|
Mentor: josh
Whiteboard: [lang=c++]
Comment 3•9 years ago
|
||
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•9 years ago
|
Mentor: josh
Whiteboard: [lang=c++]
Comment 4•9 years ago
|
||
Hi I'd like to work on this bug. Could you give me some pointers on how to fix this?
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 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•9 years ago
|
Assignee: nobody → diorahman
Comment 8•9 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•9 years ago
|
||
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 10•9 years ago
|
||
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•9 years ago
|
||
Update as suggested by Josh.
Attachment #8555230 -
Attachment is obsolete: true
Attachment #8557031 -
Flags: review?(hsivonen)
Assignee | ||
Updated•9 years ago
|
Attachment #8557031 -
Flags: review?(hsivonen) → review?(bzbarsky)
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=434cde5f13b0
Keywords: checkin-needed
Comment 14•9 years ago
|
||
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 16•9 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•9 years ago
|
||
./mach web-platform-tests
Comment 18•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 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 22•9 years ago
|
||
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+
Updated•9 years ago
|
Keywords: checkin-needed
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1660c10f980b
Keywords: checkin-needed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1660c10f980b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 26•9 years ago
|
||
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.
Description
•