Closed Bug 972856 Opened 6 years ago Closed 6 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

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
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
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae95fa9d4450

Should we have a test for this?
Flags: in-testsuite?
Keywords: checkin-needed
probably
https://hg.mozilla.org/mozilla-central/rev/ae95fa9d4450
Status: NEW → RESOLVED
Closed: 6 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.