Closed Bug 972856 Opened 11 years ago Closed 11 years ago

Timestamp of last happened CC has changed from milliseconds to microseconds in GC API

Categories

(Core :: XPCOM, defect)

29 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: whimboo, Assigned: mccr8)

References

Details

(Keywords: addon-compat, regression)

Attachments

(1 file, 1 obsolete file)

As we have seen in MemChaser the duration since the last CC happened, has been changed from milliseconds to microseconds. My investigation has been shown that bug 948554 has been caused this. As Andrew said, this is not expected. As of now Firefox 29 and 30 are affected.
Version: 28 Branch → 29 Branch
Keywords: addon-compat
I don't think it needs to be tracked. Yours is one of the only addons that actually looks at this data, and you have a work around in place if I can't get it fixed in time. :)
Assignee: nobody → continuation
I think what is to blame here is the following line: http://hg.mozilla.org/mozilla-central/diff/3453305f7101/dom/base/nsJSEnvironment.cpp#l1.167 1.167 - PRTime endCCTime = PR_Now(); 1.168 + TimeStamp endCCTime = TimeStamp::Now(); The change to TimeStamp causes us to print microseconds instead of milliseconds. Should we call endCCTime.ToMilliseconds() when formatting it forJSON instead of using endCCTime directly?
Actually this is the timestamp property and not the duration one in the CC JSON string.
Summary: Duration since last CC has changed from milliseconds to microseconds in GC API → Timestamp of last happened CC has changed from milliseconds to microseconds in GC API
Attached patch Use PR_Now() for CC timestamp in JSON log. (obsolete) (deleted) — Splinter Review
Great analysis, thanks! You saved me a lot of time. If you could test this to make sure it is okay, I'd appreciate it. There's no .ToMilliseconds() for TimeStamp, and anyways, we should be using the same method for generating the time stamp for the GC and CC, so there's a consistent timeline, and I don't feel like changing the GC approach, so I am just going to change it to use the old approach.
Attachment #8376356 - Flags: review?(bugs)
Attachment #8376356 - Flags: feedback?(hskupin)
Comment on attachment 8376356 [details] [diff] [review] Use PR_Now() for CC timestamp in JSON log. Variable names are now backwards. endCCTime is for TimeStamp and endCCTimeStamp for PR_Now()
Attachment #8376356 - Flags: review?(bugs) → review-
I switched the names. You are right, it was weird before. https://tbpl.mozilla.org/?tree=Try&rev=af50cdecffa3
Attachment #8376356 - Attachment is obsolete: true
Attachment #8376356 - Flags: feedback?(hskupin)
Attachment #8376896 - Flags: review?(bugs)
That fixes it. Thank you Andrew!
Attachment #8376896 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Flags: in-testsuite?
Keywords: checkin-needed
probably
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8376896 [details] [diff] [review] Use PR_Now() for CC timestamp in JSON log. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 948554 User impact if declined: breaks some addons Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): very low, and it shouldn't affect anybody who isn't running one of these addons String or IDL/UUID changes made by this patch: none
Attachment #8376896 - Flags: approval-mozilla-aurora?
Works fine with Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140219030203 CSet: bf0e76f2a7d4. Marking as verified fixed.
Status: RESOLVED → VERIFIED
Attachment #8376896 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Works fine also with Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140221004001 CSet: 62aa405444cd
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: