Closed Bug 1444588 Opened 2 years ago Closed 2 years ago

Usage of ClearOnShutDown introduced in bug 1425462 is not safe.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: emilio, Assigned: tjr)

References

Details

(Keywords: csectype-race, sec-moderate)

Attachments

(1 file)

I just got one of these in a try run of mine:

  https://treeherder.mozilla.org/logviewer.html#?job_id=167151568&repo=try&lineNumber=9952

looks related to the work done in bug 1425462... ClearOnShutdown is supposed to be main-thread only, and IIUC this can run from arbitrary threads:

  https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/toolkit/components/resistfingerprinting/nsRFPService.cpp#300

Which means ClearOnShutdown can race.
Flags: needinfo?(tom)
Should be fixed in the first patch of Bug 1443943 - should land that next week; unless you think it needs a fix ASAP.
Flags: needinfo?(tom)
(In reply to Tom Ritter [:tjr] from comment #1)
> Should be fixed in the first patch of Bug 1443943 - should land that next
> week; unless you think it needs a fix ASAP.

This is causing a bunch of intermittents in browser tests, but yeah, if that lands soon I guess it's fine... One problem is, I guess that means that the race would be shipped in 60, maybe we should avoid that.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> (In reply to Tom Ritter [:tjr] from comment #1)
> > Should be fixed in the first patch of Bug 1443943 - should land that next
> > week; unless you think it needs a fix ASAP.
> 
> This is causing a bunch of intermittents in browser tests, but yeah, if that
> lands soon I guess it's fine... One problem is, I guess that means that the
> race would be shipped in 60, maybe we should avoid that.

There are a collection of Jitter bugs that are going to be uplifted to Beta, this is one of them.
Group: core-security → dom-core-security
Blocks: 1439875
We're going to land the first patch in bug 1443943 separately in this bug.
Blocks: 1443943
Assignee: nobody → tom
Before, we would initialize LRUCache on the first instance of
calling the Timer Precision Reduction functions. We would both
allocate and initialize it, and call ClearOnShutdown.

ClearOnShutdown can only be called on the Main Thread, but it
just so happened that we always did that, so there was no
problem. Now that we are not calling precision reduction for
system callers, we were initializing on a non-main-thread and
we need to avoid that.

In the future, we could reduce memory use IF we are not using
the timer precision reduction functions by figuring out how
to initialize this lazily but still on the main thread. For
now, because we are using the timer precision reduction
functions, doing so would not save us any memory.
Attachment #8958524 - Flags: review+
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(tom)
Comment on attachment 8958524 [details] [diff] [review]
Move LRUCache Initialization to startup rather than lazily. r=baku

Approval Request Comment
[Feature/Bug causing the regression]: The initial version of Jitter that landed (Bug 
1425462) had a bug.

[User impact if declined]: Crashes.

[Is this code covered by automated tests?]: Indirectly, yes. In fact, they caught it.

[Has the fix been verified in Nightly?]: Yes, it's baked for a few days

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: This blocks Bug 1443943 and Bug 1440195 which we also need to uplift.

[Is the change risky?]: No.

[Why is the change risky/not risky?]: There is always a risk I got the patch wrong, but it's been reviewed and people seem to think it's good.

[String changes made/needed]: None
Flags: needinfo?(tom)
Attachment #8958524 - Flags: approval-mozilla-beta?
Comment on attachment 8958524 [details] [diff] [review]
Move LRUCache Initialization to startup rather than lazily. r=baku

sec fix, needed for other uplifts, beta60+
Attachment #8958524 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.