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)
Tracking
()
VERIFIED
FIXED
mozilla30
People
(Reporter: whimboo, Assigned: mccr8)
References
Details
(Keywords: addon-compat, regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
smaug
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Version: 28 Branch → 29 Branch
Reporter | ||
Updated•11 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 1•11 years ago
|
||
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. :)
Reporter | ||
Comment 2•11 years ago
|
||
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?
Reporter | ||
Comment 3•11 years ago
|
||
With that I mean here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#2286
Reporter | ||
Comment 4•11 years ago
|
||
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
Assignee | ||
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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-
Assignee | ||
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
That fixes it. Thank you Andrew!
Updated•11 years ago
|
Attachment #8376896 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae95fa9d4450
Should we have a test for this?
Flags: in-testsuite?
Keywords: checkin-needed
Assignee | ||
Comment 10•11 years ago
|
||
probably
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee | ||
Comment 12•11 years ago
|
||
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?
Reporter | ||
Comment 13•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #8376896 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•11 years ago
|
||
Reporter | ||
Comment 15•11 years ago
|
||
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.
Description
•