Closed Bug 1432429 (fuzzyfox) Opened 2 years ago Closed 11 months ago

[META] FuzzyFox meta-bug

Categories

(Core :: Security, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: baku, Assigned: tjr)

References

(Depends on 4 open bugs)

Details

(Keywords: meta)

Attachments

(7 files, 81 obsolete files)

2.38 KB, text/plain
chutten
: review+
Details
3.04 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
9.41 KB, patch
tjr
: review+
Details | Diff | Splinter Review
1.63 KB, patch
tjr
: review+
Details | Diff | Splinter Review
24.18 KB, patch
tjr
: review+
Details | Diff | Splinter Review
18.39 KB, patch
tjr
: review+
Details | Diff | Splinter Review
6.43 KB, patch
tjr
: review+
Details | Diff | Splinter Review
This is a meta-bug for the FuzzyFox implementation.
The current implementation can be found as a proof of concept here: https://github.com/dkohlbre/gecko-dev/commits/fuzzyfox
Alias: fuzzyfox
See Also: → 1445726
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Version: 58 Branch → Trunk
Attached patch part 0 - --enable-fuzzyfox (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attached patch part 1 - PauseTask (obsolete) — Splinter Review
Attached patch part 2 - TimeStamp::NowFuzzy (obsolete) — Splinter Review
Attached patch part 3 - Performance API (obsolete) — Splinter Review
Attached patch part 4 - DelayHttpChannelQueue (obsolete) — Splinter Review
Attached patch part 6 - Layout assertions (obsolete) — Splinter Review
Attached patch part 7 - JS fuzzy time update (obsolete) — Splinter Review
I just want to share what I have. This is, basically, a write of fuzzyfox, inspired by the github fuzzyfox project.

It introduces an --enable-fuzzyfox option to compile FuzzyFox enabled/disabled.
It currently works on posix (linux is what I'm using) because of some time syscalls that have to be ported to cross-platform functions. But this part is unfinished and it's not in this set of patches.

Something else I want to see is to check how many tests are broken with fuzzyfox enabled. I'll write comments when I have the first results.
Attached patch part 0 - --enable-fuzzyfox (obsolete) — Splinter Review
Attachment #8963659 - Attachment is obsolete: true
Attached patch part 1 - FuzzyfoxRunnable (obsolete) — Splinter Review
Attachment #8963660 - Attachment is obsolete: true
Attached patch part 1 - FuzzyfoxRunnable (obsolete) — Splinter Review
This works cross platform thanks to std c11.
Attachment #8964101 - Attachment is obsolete: true
Attachment #8964100 - Flags: review?(mh+mozilla)
Attached patch part 1 - FuzzyfoxRunnable (obsolete) — Splinter Review
Here the first patch: here we have a runnable that is scheduled on main-thread at startup time. This is similar to your PauseTask runnable, but instead being added to nsThreadPool, it's scheduled in the SystemGroup.

Another difference is that I use std c11 to calculate the monotonic and realtime clocks.
Attachment #8964113 - Attachment is obsolete: true
Attachment #8964115 - Flags: feedback?(dkohlbre)
Comment on attachment 8963661 [details] [diff] [review]
part 2 - TimeStamp::NowFuzzy

Luke, do you know who can help with the review of this patch?
Flags: needinfo?(luke)
Attachment #8963663 - Flags: feedback?(dkohlbre)
Attachment #8963665 - Flags: feedback?(dkohlbre)
Attachment #8963667 - Flags: feedback?(luke)
Attachment #8963667 - Flags: feedback?(dkohlbre)
I think Waldo has done a lot with time.
Flags: needinfo?(luke)
Attachment #8963667 - Flags: feedback?(luke) → feedback+
Comment on attachment 8964100 [details] [diff] [review]
part 0 - --enable-fuzzyfox

Review of attachment 8964100 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't you want this to be linux only?
Attachment #8964100 - Flags: review?(mh+mozilla)
Comment on attachment 8964115 [details] [diff] [review]
part 1 - FuzzyfoxRunnable

Review of attachment 8964115 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
We can probably remove most of the PT_ defines
#define PT_STATIC_DURATION  80
#define PT_STATIC_CLOCK_GRAIN 80
#define PT_DURATION_CENTER 100000

And turn them into one, since DURATION_CENTER should actually be the same as the grain, and STATIC_DURATION should be removed.
Having multiple knobs is a holdover from various tests we were running.

I believe at one point I had a working pull-from-prefs version that had a tunable granularity, but I seem to have misplaced that branch...
We'll want it to be easily tweaked given the recent discussions around different clock fuzzings/grains/jitters/etc.
(Also the current PT_DURATION_CENTER is setup for a 100ms clock, which makes things pretty rough to use)
Attachment #8964115 - Flags: feedback?(dkohlbre) → feedback+
Comment on attachment 8963663 [details] [diff] [review]
part 4 - DelayHttpChannelQueue

Review of attachment 8963663 [details] [diff] [review]:
-----------------------------------------------------------------

Only comment I have is that you may want to break this back out into a generic channelqueue if we intend to extend the queuing to all channel types.
I originally intended to write more of the other queues, but never got the chance.
Attachment #8963663 - Flags: feedback?(dkohlbre) → feedback+
Attachment #8963665 - Flags: feedback?(dkohlbre) → feedback+
Attachment #8963667 - Flags: feedback?(dkohlbre) → feedback+
> Didn't you want this to be linux only?

Not anymore. This works on any platform.
Comment on attachment 8964100 [details] [diff] [review]
part 0 - --enable-fuzzyfox

Review of attachment 8964100 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't you want this to be linux only?
Attachment #8964100 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #20)
> Comment on attachment 8964100 [details] [diff] [review]
> part 0 - --enable-fuzzyfox
> 
> Review of attachment 8964100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Didn't you want this to be linux only?

Disregard the comment, that was just splinter not being helpful.
It seems that we don't want to link libxul with libstdc++.
Does somebody know why we don't want to use system_clock and steady_clock from std::chrono?
Attached patch part 7 - JS fuzzy time update (obsolete) — Splinter Review
Attachment #8963667 - Attachment is obsolete: true
(In reply to Andrea Marchesini [:baku] from comment #22)
> It seems that we don't want to link libxul with libstdc++.
> Does somebody know why we don't want to use system_clock and steady_clock
> from std::chrono?

Just a guess, but it seems like Mozilla's TimeStamp implementation predates those features existing...
Attached patch part 1 - Fuzzyfox (obsolete) — Splinter Review
Attachment #8964115 - Attachment is obsolete: true
Attachment #8964958 - Flags: review?(nfroyd)
Attached patch part 2 - TimeStamp::NowFuzzy (obsolete) — Splinter Review
Attachment #8963661 - Attachment is obsolete: true
Attachment #8964959 - Flags: review?(nfroyd)
Attached patch part 7 - JS fuzzy time update (obsolete) — Splinter Review
Let's go step by step with the review. When the first to blocks are OK, I'll ask the review for the rest.
Attachment #8964549 - Attachment is obsolete: true
I have to admit that I'm not particularly happy of touching TimeStamp class. I would like to see if I can make a wrapper or a helper class (FuzzyTimeStamp) that returns a fuzzy version of Now(). Then I want to audit all the uses of TimeStamp::Now() and, #ifdef MOZ_FUZZYFOX, use FuzzyTimeStamp::Now() instead.

About PR_Now(), I could do a similar approach.
Flags: needinfo?(nfroyd)
(In reply to Andrea Marchesini [:baku] from comment #28)
> I have to admit that I'm not particularly happy of touching TimeStamp class.
> I would like to see if I can make a wrapper or a helper class
> (FuzzyTimeStamp) that returns a fuzzy version of Now(). Then I want to audit
> all the uses of TimeStamp::Now() and, #ifdef MOZ_FUZZYFOX, use
> FuzzyTimeStamp::Now() instead.
> 
> About PR_Now(), I could do a similar approach.

I think I like that a little better; I still haven't looked closely at the paper or the patch, so that's a very tentative opinion.  Except that, wait, the last sentence makes it sound like you want to do:

#ifdef MOZ_FUZZYFOX
  FuzzyTimeStamp::Now();
#else
  TimeStamp::Now();
#endif

Is that understanding correct?  If so, I don't think we should go that route. :)

Either way, my main concern is when new uses of TimeStamp need to be added, how can we make sure the correct one is being used?  Is the new "default" to always be fuzzy, or is there some other guideline?  Can that guideline be made concrete enough to have a static analysis alert people when they use the wrong one?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #29)
> Either way, my main concern is when new uses of TimeStamp need to be added,
> how can we make sure the correct one is being used?  Is the new "default" to
> always be fuzzy, or is there some other guideline?  Can that guideline be
> made concrete enough to have a static analysis alert people when they use
> the wrong one?

OK, so after reading the FuzzyFox paper, my understanding is that the answer to this is "always use the fuzzy one", and that we should set things up so that the fuzzy one is actually the "real" TimeStamp when --disable-fuzzyfox.  Is that correct?

(That guideline might botch some of our telemetry measurements and other profiling bits, but...does Tor Browser--or other people using --enable-fuzzyfox--even send telemetry to us?  Probably not.)
Comment on attachment 8964958 [details] [diff] [review]
part 1 - Fuzzyfox

Review of attachment 8964958 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look closely at the logic that Fuzzyfox implements right now.

::: toolkit/components/fuzzyfox/Fuzzyfox.cpp
@@ +56,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  long int seedv;
> +  PR_GetRandomNoise(&seedv,sizeof(long int));
> +  srand48_r(seedv, &mRandState);

I think we are going to want something a little stronger here: PR_GetRandomNoise appears to just mix bits of clocks into the given buffer (at least on Unix), which is...not ideal.  My inclination is to also use something stronger than an LCG for the random number generator.

@@ +160,5 @@
> +    os->NotifyObservers(nullptr, "fuzzyfox-fire-outbound", nullptr);
> +  }
> +
> +  nsCOMPtr<nsISupportsPRInt64> wrapper =
> +    do_CreateInstance(NS_SUPPORTS_PRINT64_CONTRACTID);

We should create one of these, stored in the Fuzzyfox instance, and re-use it every time.

::: toolkit/components/fuzzyfox/Fuzzyfox.h
@@ +10,5 @@
> +#include "nsThreadUtils.h"
> +
> +namespace mozilla {
> +
> +class Fuzzyfox final : public Runnable

This class *definitely* needs some documentation.  The various methods in this class need documentation, too.
Attachment #8964958 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8964959 [details] [diff] [review]
part 2 - TimeStamp::NowFuzzy

Review of attachment 8964959 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling, waiting for discussion on comment 29 and comment 30.
Attachment #8964959 - Flags: review?(nfroyd)
I took a look at the boot-time calculation re-work, and it looks like it will need tweaks.

The current patch calculates canonical time with:
>145	  int64_t time = ActualTime();
>146	  int64_t newTime = RoundToGrain(time);
>147	  int64_t newTimeStamp = US_TO_NS(RoundToGrain(TimeStamp::NowReally().mValue));

The problem here (I believe) is that the value returned by ActualTime() and TimeStamp::NowReally().mValue will be offset from each other slightly. If these round to the same value its not a problem.
However, if they round to different values since they straddle a multiple of the grain, all the fuzzytime guarantees can fall down.

We'll need to find a way to ensure that newTime and newTimeStamp represent the same multiple of the granularity no matter what. (that is newTimeStamp being newTime + bootTime and never newTime + bootTime + grain)
> #ifdef MOZ_FUZZYFOX
>   FuzzyTimeStamp::Now();
> #else
>   TimeStamp::Now();
> #endif

Yes. I gave a try but I'm not particularly enthusiastic of a FuzzyTimeStamp class.
Probably we should stay with what we have.

I'm going to apply the initial comments, but, in the meantime, do you want to continue the review?
Flags: needinfo?(nfroyd)
(In reply to Andrea Marchesini [:baku] from comment #34)
> > #ifdef MOZ_FUZZYFOX
> >   FuzzyTimeStamp::Now();
> > #else
> >   TimeStamp::Now();
> > #endif
> 
> Yes. I gave a try but I'm not particularly enthusiastic of a FuzzyTimeStamp
> class.
> Probably we should stay with what we have.
> 
> I'm going to apply the initial comments, but, in the meantime, do you want
> to continue the review?

I'm confused about what continuing I have to do here.  I'm not sure we have an answer to the question raised in comment 30.
Flags: needinfo?(nfroyd)
> OK, so after reading the FuzzyFox paper, my understanding is that the answer
> to this is "always use the fuzzy one", and that we should set things up so
> that the fuzzy one is actually the "real" TimeStamp when --disable-fuzzyfox.
> Is that correct?

Yes. But it's the other-way around: We want fuzzyfox to be disabled by default. So, when enabled, TimeStamp::Now() returns a fuzzy time.

> (That guideline might botch some of our telemetry measurements and other
> profiling bits, but...does Tor Browser--or other people using
> --enable-fuzzyfox--even send telemetry to us?  Probably not.)

I can disable telemetry when fuzzyfox is enabled. Wondering how accurate telemetry is.
Flags: needinfo?(nfroyd)
(In reply to Andrea Marchesini [:baku] from comment #36)
> > OK, so after reading the FuzzyFox paper, my understanding is that the answer
> > to this is "always use the fuzzy one", and that we should set things up so
> > that the fuzzy one is actually the "real" TimeStamp when --disable-fuzzyfox.
> > Is that correct?
> 
> Yes. But it's the other-way around: We want fuzzyfox to be disabled by
> default. So, when enabled, TimeStamp::Now() returns a fuzzy time.

OK, we are on the same page, then.  That will be nice for not messing with blame as well.

> > (That guideline might botch some of our telemetry measurements and other
> > profiling bits, but...does Tor Browser--or other people using
> > --enable-fuzzyfox--even send telemetry to us?  Probably not.)
> 
> I can disable telemetry when fuzzyfox is enabled. Wondering how accurate
> telemetry is.

Forcibly disabling telemetry sounds like the right way forward, at least at first.
Flags: needinfo?(nfroyd)
Attached patch part 2 - TimeStamp::NowFuzzy (obsolete) — Splinter Review
rebased
Attachment #8964959 - Attachment is obsolete: true
rebased
Attachment #8963665 - Attachment is obsolete: true
Attached patch part 7 - JS fuzzy time update (obsolete) — Splinter Review
Attachment #8964961 - Attachment is obsolete: true
Duplicate of this bug: 1464565
Depends on: 1473714
Hey baku, I think this is ready for you to look at, at a high level at least.

Patch 2/14 is your patch 2 modified to fix Windows. 1 and 3-8 are your unmodified patches and 9-14 are mine on top.

There's two thing I'd don't care for:

1) I replicate the mt2ms macros from Timestamp_windows.cpp into Fuzzyfox.cpp.  I tried moving them into Timestamp_windows.h but sFrequencyPerSec needs to be accessible... and Timestamp gets compiled into mozglue while Fuzzyfox is in xul. I acan fix this with an exported function though so I'm goig to try that.

2) There are three booleans containing whether fuzzyfox is enabled: in Timestamp, in Fuzzyfox, and in the JS Runtime. I'm going to try and collapse them all to just one. I can fix the JS Runtime by passing in a callback I think (so you can completely ignore 13/14 while I work on this). As for Fuzzyfox/Timestamp  I'm going to try moving the Pref-Tracking Atomic into Timestamp but if that doesn't work I'll just have a regular bool and update it on observed pref change from Fuzzyfox.

I also haven't addressed any other feedback given in the bug...
Flags: needinfo?(amarchesini)
Attachment #8990365 - Attachment is obsolete: true
Attachment #8990366 - Attachment is obsolete: true
Attachment #8990367 - Attachment is obsolete: true
Attachment #8990368 - Attachment is obsolete: true
Attachment #8990369 - Attachment is obsolete: true
Attachment #8990370 - Attachment is obsolete: true
Attachment #8990371 - Attachment is obsolete: true
Attachment #8990372 - Attachment is obsolete: true
Attachment #8990373 - Attachment is obsolete: true
Attachment #8990374 - Attachment is obsolete: true
Attachment #8990375 - Attachment is obsolete: true
Attachment #8990376 - Attachment is obsolete: true
Attachment #8990377 - Attachment is obsolete: true
(In reply to Tom Ritter [:tjr] from comment #72)
> Hey baku, I think this is ready for you to look at, at a high level at least.
> 
> Patch 2/14 is your patch 2 modified to fix Windows. 1 and 3-8 are your
> unmodified patches and 9-14 are mine on top.
> 
> There's two thing I'd don't care for:
> 
> 1) I replicate the mt2ms macros from Timestamp_windows.cpp into
> Fuzzyfox.cpp.  I tried moving them into Timestamp_windows.h but
> sFrequencyPerSec needs to be accessible... and Timestamp gets compiled into
> mozglue while Fuzzyfox is in xul. I acan fix this with an exported function
> though so I'm goig to try that.

Fixed this.

> 2) There are three booleans containing whether fuzzyfox is enabled: in
> Timestamp, in Fuzzyfox, and in the JS Runtime. I'm going to try and collapse
> them all to just one. I can fix the JS Runtime by passing in a callback I
> think (so you can completely ignore 13/14 while I work on this). As for
> Fuzzyfox/Timestamp  I'm going to try moving the Pref-Tracking Atomic into
> Timestamp but if that doesn't work I'll just have a regular bool and update
> it on observed pref change from Fuzzyfox.

I fixed this also, but I want a JS reviewer to give feedback (both coarse and fine).
Comment on attachment 8991424 [details]
Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled 13/14

https://reviewboard.mozilla.org/r/256324/#review263202

::: js/src/jsapi.h:5498
(Diff revision 1)
>  
> +typedef bool
> +(* CheckLockedClockEnabledStatusCallback)();
> +
> +extern JS_PUBLIC_API(void)
> +JS_InitCheckLockedClockEnabledStatusCallback(JSContext* cx, CheckLockedClockEnabledStatusCallback callback);

Should this be JS(underscore) or should it be unprefixed in the JS namespace?

::: js/src/vm/FuzzyfoxClock.h:13
(Diff revision 1)
>  #define FuzzyfoxClock_h
>  
>  namespace js {
>  
>  /* Locked clock impl*/
>  class FuzzyfoxClock

The instance of this class is being stored on the Runtime; but every runtime will have the same value for the currentClock, and every callback will be the same.  Can we move it so it's not duplicated?
(In reply to Tom Ritter [:tjr] from comment #89)
> Comment on attachment 8991424 [details]
> Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled
> 13/14
> 
> https://reviewboard.mozilla.org/r/256324/#review263202
> 
> ::: js/src/jsapi.h:5498
> (Diff revision 1)
> >  
> > +typedef bool
> > +(* CheckLockedClockEnabledStatusCallback)();
> > +
> > +extern JS_PUBLIC_API(void)
> > +JS_InitCheckLockedClockEnabledStatusCallback(JSContext* cx, CheckLockedClockEnabledStatusCallback callback);
> 
> Should this be JS(underscore) or should it be unprefixed in the JS namespace?
> 
> ::: js/src/vm/FuzzyfoxClock.h:13
> (Diff revision 1)
> >  #define FuzzyfoxClock_h
> >  
> >  namespace js {
> >  
> >  /* Locked clock impl*/
> >  class FuzzyfoxClock
> 
> The instance of this class is being stored on the Runtime; but every runtime
> will have the same value for the currentClock, and every callback will be
> the same.  Can we move it so it's not duplicated?

Spoke with tcampbell:

 - It might make more sense to put into jsfriendapi instead (and js:: namespace)
 - JSRuntime is a good place to keep this; but I could make all instances point to the same one
Comment on attachment 8991413 [details]
Bug 1432429 Add FuzzyFox class 2/14

https://reviewboard.mozilla.org/r/256302/#review266282

The window part looks good. But maybe ask froydnj to take a look too.
Attachment #8991413 - Flags: review+
Comment on attachment 8991421 [details]
Bug 1432429 Improve the macros, logging, and comments in Fuzzyfox.cpp 10/14

https://reviewboard.mozilla.org/r/256318/#review266284

::: toolkit/components/fuzzyfox/Fuzzyfox.cpp:75
(Diff revision 1)
>  
>  Fuzzyfox::~Fuzzyfox() = default;
>  
>  NS_IMETHODIMP
> +
> +#define QUEUE_AND_RETURN() \

maybe DISPATCH_AND_RETURN ?

Are you planning to use it elsewhere?
Attachment #8991421 - Flags: review+
Comment on attachment 8991420 [details]
Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14

https://reviewboard.mozilla.org/r/256316/#review266286
Attachment #8991420 - Flags: review+
Comment on attachment 8991422 [details]
Bug 1432429 Switch Fuzzyfox over to a pref 11/14

https://reviewboard.mozilla.org/r/256320/#review266288

It looks good! Ask froydnj a second review for the window clock part (and in general TimeStamp).

::: mozglue/misc/TimeStamp.h:610
(Diff revision 1)
>    static MFBT_API void Shutdown();
>  
>  private:
>    friend struct IPC::ParamTraits<mozilla::TimeStamp>;
>  
> +  static bool sFuzzyfoxEnabled;

you can move this into TimeStamp.cpp

::: mozglue/misc/TimeStamp.cpp:52
(Diff revision 1)
>  };
>  
> +bool TimeStamp::sFuzzyfoxEnabled;
> +
> +/* static */ bool
> +TimeStamp::GetFuzzyfoxEnabled() 

extra space here

::: toolkit/components/fuzzyfox/Fuzzyfox.cpp:153
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  if (!TimeStamp::GetFuzzyfoxEnabled()) {
> +    LOG(Info, ("[FuzzyfoxEvent] PT(%p) Fuzzyfox is shut down, doing nothing \n", this));
> +    return NS_OK;
> +  } else if (mStartTime == 0) {

no }else { after a return. Just do:

}

if (mStartTime == 0) { ..

::: toolkit/components/fuzzyfox/Fuzzyfox.cpp:186
(Diff revision 1)
> +    LOG(Debug, ("[FuzzyfoxEvent] PT(%p) endTime < mStartTime mStartTime %" PRIu64 " endTime %" PRIu64 " \n",
> +         this, mStartTime, endTime));
> +
> +    mSanityCheck = true;
> +    QUEUE_AND_RETURN();
> +  }

same here.

no } else ..
Attachment #8991422 - Flags: review+
Comment on attachment 8991423 [details]
Bug 1432429 Adjust DelayQueueHTTPChannel based on whether or not Fuzzyfox is enabled 12/14

https://reviewboard.mozilla.org/r/256322/#review266290
Attachment #8991423 - Flags: review+
Comment on attachment 8990378 [details]
Bug 1432429 Refactor Timestamp to have a NowInternal function 14/14

https://reviewboard.mozilla.org/r/255458/#review266294

good to me. But I'm not a peer of this component.
Attachment #8990378 - Flags: review+
Comment on attachment 8991417 [details]
Bug 1432429  Worker fuzzy clock update 6/14

https://reviewboard.mozilla.org/r/256310/#review267396

::: dom/workers/WorkerPrivate.cpp:2225
(Diff revision 1)
> +  AssertIsOnParentThread();
> +
> +  RefPtr<UpdateFuzzyFoxClockRunnable> runnable =
> +    new UpdateFuzzyFoxClockRunnable(this, aNewTime);
> +  if (!runnable->Dispatch()) {
> +    NS_WARNING("Failed to dispatch fuzzy fox clock update event!");

This frequently happens, and spews warnings like the following:

[Parent 90934, Main Thread] WARNING: Failed to dispatch fuzzy fox clock update event!: file /Users/tritter/Documents/moz/mozilla-fuzzyfox/dom/workers/WorkerPrivate.cpp, line 2222
[Parent 90934, Main Thread] WARNING: A runnable was posted to a worker that is already shutting down!: file /Users/tritter/Documents/moz/mozilla-fuzzyfox/dom/workers/WorkerPrivate.cpp, line 1648
Attachment #8997528 - Flags: review?(chutten)
Outstanding issues:

 - We need a better (secure) RNG.  Unlike ReduceTimerPrecision, this RNG is going to need to generate data every time slice, so it needs to be as fast as we can make it.
 - [done] The comment from Nathan in #31 about reusing wrapper  (Done in commit 11)
 - [done] David's comments in Comment #17  (Done in commit 11)
 - David's comments in Comment #33
 - [done] Adjust Telemetry.   (Done in commit 15)
 - Documentation.


(In reply to David Kohlbrenner from comment #33)
> I took a look at the boot-time calculation re-work, and it looks like it
> will need tweaks.
> 
> The current patch calculates canonical time with:
> >145	  int64_t time = ActualTime();
> >146	  int64_t newTime = RoundToGrain(time);
> >147	  int64_t newTimeStamp = US_TO_NS(RoundToGrain(TimeStamp::NowReally().mValue));
> 
> The problem here (I believe) is that the value returned by ActualTime() and
> TimeStamp::NowReally().mValue will be offset from each other slightly. If
> these round to the same value its not a problem.
> However, if they round to different values since they straddle a multiple of
> the grain, all the fuzzytime guarantees can fall down.
> 
> We'll need to find a way to ensure that newTime and newTimeStamp represent
> the same multiple of the granularity no matter what. (that is newTimeStamp
> being newTime + bootTime and never newTime + bootTime + grain)

So yes, they will be offset from one another slightly. But resolving this might be difficult.

ActualTime is microseconds since epoch.

Timestamp::NowReally() is:
1) On Mac: mach_absolute_time() aka nanoseconds since boot
2) On Windows: Either QueryPerformanceCounter or GetTickCount64() (milliseconds since boot)
3) On Linux: CLOCK_MONOTONIC clock_gettime() which is 'monotonic time since some unspecified starting point'

At first, I thought we would never expose TimeStamp values explicitly or implicitly to content; since they have no meaningful value - but I forgot that in a lot of places we take the difference between two timestamps, convert that to milliseconds, and expose that.

I started thinking about if we could detect if one was rounded up and the other was rounded down - but then I realized - these are on different timelines; aren't they?  e.g. if a TimeStamp's absolute value is at 5 and the current time is 10 - with a grain of 10 - isn't this going to be all screwy?

David - maybe I need to talk through this with you in a meeting about what we could do and what is safe...
Assignee: amarchesini → tom
Flags: needinfo?(amarchesini) → needinfo?(dkohlbre)
Priority: P2 → P1
Comment on attachment 8997525 [details]
Bug 1432429 Record the fuzzyfox pref in Telemetry 15/14

https://reviewboard.mozilla.org/r/261248/#review268326

One question only.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:256
(Diff revision 1)
>    ["network.proxy.ssl", {what: RECORD_PREF_STATE}],
>    ["pdfjs.disabled", {what: RECORD_PREF_VALUE}],
>    ["places.history.enabled", {what: RECORD_PREF_VALUE}],
>    ["plugins.remember_infobar_dismissal", {what: RECORD_PREF_VALUE}],
>    ["plugins.show_infobar", {what: RECORD_PREF_VALUE}],
> +  ["privacy.fuzzyfox.enabled", {what: RECORD_PREF_VALUE}],

What values of privacy.fuzzyfox.enabled are expected and in what mix?
Attachment #8997525 - Flags: review?(chutten) → review+
Comment on attachment 8997528 [details]
fuzzyfox-data-collection-request.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? 

Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

:tjr, do you commit to monitoring this data over time?

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1

    Is the data collection request for default-on or default-off?

Default-on

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No.

---
Result: datareview+, pending :tjr's answer to the monitoring question.
Flags: needinfo?(tom)
Attachment #8997528 - Flags: review?(chutten) → review+
(In reply to Chris H-C :chutten from comment #116)
> What values of privacy.fuzzyfox.enabled are expected and in what mix?

99.99% false. .01% true. Probably less.

(In reply to Chris H-C :chutten from comment #117)
>     If the request is for permanent data collection, is there someone who
> will monitor the data over time?**
> 
> :tjr, do you commit to monitoring this data over time?

Well, the primary goal of this collection is to enable you to exclude fuzzyfox data so that it won't pollute the general pool.  I can commit to performing an analysis of "Does Fuzzyfox introduce telemetry differences that require continuing to segment it" if we pursue enabling Fuzzyfox by default. (Which I hope we will but will be dependent on performance review of the feature...)  If the result of that is "It doesn't" we can remove it. If it's "It does" then we'll want to segment it or change things so it doesn't (and then remove it).
Flags: needinfo?(tom)
Sounds fine by me.
Depends on: 1484298
Depends on: 1484299
Depends on: 1484300
This will eventually be removed, but we keep this commit to
preserve the original patch series.
Creates a FuzzyFox class for implementating the core of the step/sleep
algorithm.

Moves the ms2mt macros from TimeStamp_windows.cpp to TimeStamp_windows.h and
creates a new public function GetQueryPerformanceFrequencyPerSec() to expose
a static variable in the .cpp file. This is necessary to support the macros
being usable anywhere.  (And we use the macros in the FuzzyFox.)

Depends on D3626
TimeStamp::Now will now return a Fuzzy value.
We add a NowReally function to support obtaining the real timestamp.

Depends on D3627
Attachment #8963662 - Attachment is obsolete: true
Attachment #8963663 - Attachment is obsolete: true
Attachment #8963666 - Attachment is obsolete: true
Attachment #8964100 - Attachment is obsolete: true
Attachment #8964958 - Attachment is obsolete: true
Attachment #8979445 - Attachment is obsolete: true
Attachment #8979446 - Attachment is obsolete: true
Attachment #8979448 - Attachment is obsolete: true
Attachment #8990378 - Attachment is obsolete: true
Attachment #8991412 - Attachment is obsolete: true
Attachment #8991413 - Attachment is obsolete: true
Attachment #8991414 - Attachment is obsolete: true
MozReview-Commit-ID: 7nORNl67WKG

Depends on D3628
Attachment #8991415 - Attachment is obsolete: true
Attachment #8991416 - Attachment is obsolete: true
Attachment #8991417 - Attachment is obsolete: true
Attachment #8991418 - Attachment is obsolete: true
Attachment #8991419 - Attachment is obsolete: true
Attachment #8991420 - Attachment is obsolete: true
Attachment #8991421 - Attachment is obsolete: true
Attachment #8991422 - Attachment is obsolete: true
Attachment #8991423 - Attachment is obsolete: true
Attachment #8991424 - Attachment is obsolete: true
Attachment #8997525 - Attachment is obsolete: true
In a later patch we will update the main JS Runtime for a document,
which runs in the same thread. In this patch we create the infrastructure
for updating a Worker's JS Runtime, which is seperate from the main runtime.

The Runtime service listens for fuzzyfox-update-clocks and when it observes
it, calls SendFuzzyFoxClockToAllWorkers. In turn,
SendFuzzyFoxClockToAllWorkers will use the BROADCAST_ALL_WORKERS macro to
call UpdateFuzzyFoxClock on all Workers.

UpdateFuzzyFoxClock will dispatch a new Runnable,
UpdateFuzzyFoxClockRunnable, which will finally call JS_UpdateLockedClock.

JS_UpdateLockedClock doesn't exist quite yet, but when it does, it will
update the time in the JS Runtime.

Depends on D3632
Additionally, address an early-on assertion to ensure that fuzzy and
non-fuzzy timestamps are not compared.

Depends on D3634
This patch exposes a function for the JS Runtime JS_UpdateLockedClock.
The now-fuzzed time will start there and snake through a bunch of
functions  until it is finally stored and then actually used in
PRMJ_Now.

JS_UpdateLockedClock calls cx->runtime()->updateLockedClock.
JSRuntime::updateLockedClock calls updateFuzzyfoxClock on a member
variable of type FuzzyfoxClock.

As indicated in an earlier patch, JS_UpdateLockedClock is called
for Workers to update their Runtimes.  But that doesn't update the
main Runtime.  To update the main Runtime we introduce an additional
class in this patch: FuzzyfoxClockUpdater.

FuzzyfoxClockUpdater is a member variable on XPCJSRuntime and also
listens for the fuzzyfox-update-clocks topic. When it observes it,
it will call JS_UpdateLockedClock to update the main Runtime.

Depends on D3638
MozReview-Commit-ID: 28abz1Lm0Qv

Depends on D3641
This introduces a static bool on the TimeStamp class, sFuzzyfoxEnabled,
that is the authoritative source of truth or whether FuzzyFox is enabled
or disabled, and the entire project should use it.

FuzzyFox sets itself up as an nsIObserver to watch for pref changes.
If it sees FuzzyFox being enabled/disabled it tells the TimeStamp class
to update that boolean.

With Fuzzyfox enabled unconditionally, everything is passing through
TimeStamp's NowFuzzy so we update that to check the boolean to determine
whether to return a fuzzy or non-fuzzy timestamp.
(Unfortunately, we have to add an additional condition check even when
FuzzyFox is disabled, but I don't see a way around that.)

We rename RoundGrain to the more accurate FloorToGrain, as well as
add CeilToGrain.  Normally we will use FloorToGrain, but when we first
enable FuzzyFox via the pref, we use CeilToGrain to avoid time going
backwards.

We also go through and are more explicit about unit conversions.

We take this opportunity to collapse the two FuzzyFox knobs (clock grain
and duration center) into a single one, as well as a member variable for
reusing the nsISupportsPRInt64 we use in time update topic broadcasts.

Depends on D3643
MozReview-Commit-ID: FEEgXDRpvxG

Depends on D3646
This patch brings the pref logic into the JS Runtime.

First we create a public non-member function GetFuzzyfoxEnabled() that
wraps TimeStamp::GetFuzzyfoxEnabled() so it can be used as a JS Callback.

Then we create a js::InitCheckLockedClockEnabledStatusCallback function
that we call at all Runtime Initializations (shell, nsJSEnvironment) with
the aforementioned function.

We update FuzzyfoxClock to call our callback to determine if FuzzyFox is
enabled. And then we switch PRMJ_Now over so it will return a Fuzzy Time
only if FuzzyFox is enabled. (The same, ugly, additional conditional remains
if FuzzyFox is not enabled =/ )

We also make the FuzzyfoxClock instance on the JS Runtime static, since
we will never have differest values for different Runtimes.

Finally, we move our external facing functions from jsapi over to jsfriendapi

Depends on D3647
While not strictly necessary, the Windows-specific TimeStamp
code duplicated some logic. We unified this into a single function
and replicated the same design for Posix/Mac even though they
did not duplicate code.

Depends on D3648
This will allow us to differentiate Telemetry pings with Fuzzyfox
and those without, so we don't cause bad data from Fuzzyfox to propagate.

MozReview-Commit-ID: C0FmxateaeU

Depends on D3649
Attached patch all-fuzzyfox.patch (obsolete) — Splinter Review
Attached is a patch that collapsed all the patches submitted for review so one can easily see the final changes.
Comment on attachment 9002057 [details]
Bug 1432429 Record the fuzzyfox pref in Telemetry 15/14 r=baku

Chris H-C :chutten has approved the revision.
Attachment #9002057 - Flags: review+
Creates a FuzzyFox class for implementating the core of the step/sleep
algorithm. Starts it in nsLayoutStatics::Initialize()

Adds the fuzzyfox prefs.

Moves the ms2mt macros from TimeStamp_windows.cpp to TimeStamp_windows.h and
creates a new public function GetQueryPerformanceFrequencyPerSec() to expose
a static variable in the .cpp file. This is necessary to support the macros
being usable anywhere.  (And we use the macros in the FuzzyFox.)
Creates GetFuzzyfoxEnabled() functions that check a static boolean.
Exposes SetFuzzyfoxEnabled() because we cannot depend on Pref
Observation code inside the TimeStamp class.

TimeStamp::Now will now return a Fuzzy value.

We add a NowReally function to support obtaining the real timestamp.

We also add a UsedCanonicalNow to expose whether the TimeStamp was
real or fuzzy.

Depends on D5316
1: Correct the Performance API 'NowUnclamped' to obtain a non-Fuzzed Timestamp

2: Do not compare fuzzed and non-fuzzed timestamps in a Refresh Driver assert,
since this happens so early on that we will eventually compare them and
asserting would be bad.

Depends on D5319
Attachment #9002032 - Attachment is obsolete: true
Attachment #9002033 - Attachment is obsolete: true
Attachment #9002034 - Attachment is obsolete: true
Attachment #9002035 - Attachment is obsolete: true
Attachment #9002038 - Attachment is obsolete: true
Attachment #9002040 - Attachment is obsolete: true
Attachment #9002044 - Attachment is obsolete: true
Attachment #9002046 - Attachment is obsolete: true
Attachment #9002047 - Attachment is obsolete: true
Attachment #9002049 - Attachment is obsolete: true
Attachment #9002052 - Attachment is obsolete: true
Attachment #9002054 - Attachment is obsolete: true
Attachment #9002055 - Attachment is obsolete: true
Attachment #9002056 - Attachment is obsolete: true
Attachment #9002057 - Attachment is obsolete: true
Attachment #9002083 - Attachment is obsolete: true
In a later patch we will update the main JS Runtime for a document,
which runs in the same thread. In this patch we create the infrastructure
for updating a Worker's JS Runtime, which is seperate from the main runtime.

The Runtime service listens for fuzzyfox-update-clocks and when it observes
it, calls SendFuzzyFoxClockToAllWorkers. In turn,
SendFuzzyFoxClockToAllWorkers will use the BROADCAST_ALL_WORKERS macro to
call UpdateFuzzyFoxClock on all Workers.

UpdateFuzzyFoxClock will dispatch a new Runnable,
UpdateFuzzyFoxClockRunnable, which will finally call js::UpdateLockedClock.

js::UpdateLockedClock doesn't exist quite yet, but when it does, it will
update the time in the JS Runtime.

Depends on D5324
This patch exposes a function for the JS Runtime: js::UpdateLockedClock.
The now-fuzzed time will start there and snake through a bunch of
functions until it is finally stored and then actually used in
PRMJ_Now.

js::UpdateLockedClock calls cx->runtime()->updateLockedClock.
JSRuntime::updateLockedClock calls fuzzyfoxClock->updateFuzzyfoxClock
where fuzzyfoxClock is a static member variable of a JSRuntime.

As indicated in an earlier patch, js::UpdateLockedClock is called
for Workers to update their Runtimes. But that doesn't update the
main Runtime. To update the main Runtime we introduce an additional
class in this patch: FuzzyfoxClockUpdater.

FuzzyfoxClockUpdater is a member variable on XPCJSRuntime and also
listens for the fuzzyfox-update-clocks topic. When it observes it,
it will call js::UpdateLockedClock to update the main Runtime.

Finally, to support Fuzzyfox being enabled/disabled, we also expose
js::InitCheckLockedClockEnabledStatusCallback which accepts a pointer
to a callback function that goes into the TimeStamp class to ask if
Fuzzyfox is enabled or not. Checking for that is part of
hasValidFuzzyfoxClock() which is called in all of the PRMJ_Now
functions.

Depends on D5325
This will allow us to differentiate Telemetry pings with Fuzzyfox
and those without, so we don't cause bad data from Fuzzyfox to propagate.

Depends on D5326
Comment on attachment 9007371 [details]
Bug 1432429 Record the fuzzyfox pref in Telemetry 7/7 r=chutten

Chris H-C :chutten has approved the revision.
Attachment #9007371 - Flags: review+
Comment on attachment 9007364 [details]
Bug 1432429 Clean up a few random areas of code to accomodate Fuzzyfox: Performance and RefreshDriver 3/7 r=froydnj,baku

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9007364 - Flags: review+
Comment on attachment 9007366 [details]
Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 4/7 r=froydnj,baku

Andrea Marchesini [:baku] has approved the revision.
Attachment #9007366 - Flags: review+
Comment on attachment 9007361 [details]
Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/7 r=baku,froydnj

Andrea Marchesini [:baku] has approved the revision.
Attachment #9007361 - Flags: review+
Jan, can you look at the JS reviews here so we keep it moving? They look reasonable enough to me but I'm a little busy to get into the details of it.
Flags: needinfo?(jdemooij)
Comment on attachment 9007367 [details]
Bug 1432429 Update the clock in (Javascript) Workers 5/7 r=baku

Andrea Marchesini [:baku] has approved the revision.
Attachment #9007367 - Flags: review+
Comment on attachment 9007358 [details]
Bug 1432429 Add FuzzyFox class and prefs 1/7 r=baku,froydnj

Andrea Marchesini [:baku] has approved the revision.
Attachment #9007358 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40c943cca38880b5fe1febb74e770f1d21a9450 is a try run

:jandem pointed out that the JS Engine has access to Timestamp, so couldn't we just make a global variable there and use it in the Js Engines, and avoid all the runtime stuff? I think so. I need to do some local testing, but this seems like it should work.  

:baku I was able to completely drop the Worker stuff and nearly all the JS stuff.  Nathan, I flagged you for review on that part too.
Flags: needinfo?(nfroyd)
Flags: needinfo?(amarchesini)
(In reply to Tom Ritter [:tjr] from comment #152)
> :baku I was able to completely drop the Worker stuff and nearly all the JS
> stuff.  Nathan, I flagged you for review on that part too.

This is https://phabricator.services.mozilla.com/D5326 ?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #153)
> (In reply to Tom Ritter [:tjr] from comment #152)
> > :baku I was able to completely drop the Worker stuff and nearly all the JS
> > stuff.  Nathan, I flagged you for review on that part too.
> 
> This is https://phabricator.services.mozilla.com/D5326 ?

Yes.
Attachment #9007358 - Attachment is obsolete: true
Attachment #9007361 - Attachment is obsolete: true
Attachment #9007364 - Attachment is obsolete: true
Attachment #9007366 - Attachment is obsolete: true
Attachment #9007367 - Attachment is obsolete: true
Attachment #9007369 - Attachment is obsolete: true
Attachment #9007371 - Attachment is obsolete: true
Attachment #9011870 - Flags: feedback?(dkohlbre)
Attachment #9011874 - Flags: review?(nfroyd)
Attachment #9011874 - Flags: review?(jdemooij)
Attachment #9011875 - Flags: review?(chutten)
Apologies for re-request review, yet again. I found out Phabricator was ignoring commit message updates, and had messed up the patch series after I dropped a patch, so anything complicated I'm just going to use Splinter for.

The major change between Splinter and Phabricator was I dropped all the complexity of updating workers and just check the TimeStamp class in the JS Runtime. This is in Patch 5. :baku I'd like confirmation that you're okay with that (works fine for me locally).

Patch 1: Had a r+ from baku, but I need feedback from David on one thing before I ask froydnj for a re-review.
Patch 2: baku gave a r+, would like one from froydnj
Patch 3: froydnj gave a r+ in Phabriactor
Patch 4: baku gave a r+ in Phabriactor
Patch 5: New. Needs review.
Patch 6: chutten gave a r+ in Phabriactor
Comment on attachment 9011875 [details] [diff] [review]
Bug 1432429 Record the fuzzyfox pref in Telemetry 6/6

Review of attachment 9011875 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying over the r+
Attachment #9011875 - Flags: review?(chutten) → review+
Comment on attachment 9011874 [details] [diff] [review]
Bug 1432429 Point the JS Runtimes at TimeStamp for the current time

Review of attachment 9011874 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the js/src changes with nit below addressed. Thanks!

::: js/src/vm/Time.cpp
@@ +175,5 @@
>          pGetSystemTimePreciseAsFileTime(&ft);
> +        int64_t now = int64_t(FileTimeToUnixMicroseconds(ft));
> +        // We check the FuzzyFox clock in case it was recently disabled, to prevent time
> +        // from going backwards.
> +        return mozilla::TimeStamp::NowFuzzyTime() > now ? mozilla::TimeStamp::NowFuzzyTime() : now;

This change can be reverted now.
Attachment #9011874 - Flags: review?(jdemooij) → review+
Attachment #9011874 - Attachment is obsolete: true
Attachment #9011874 - Flags: review?(nfroyd)
Attachment #9012171 - Flags: review?(nfroyd)
Attachment #9012171 - Flags: review?(jdemooij)
Attachment #9012171 - Flags: review?(jdemooij) → review+
Attachment #9002046 - Attachment is obsolete: false
Attachment #9002055 - Attachment is obsolete: false
Attachment #9002055 - Attachment is obsolete: true
Attachment #9002046 - Attachment is obsolete: true
Comment on attachment 9011873 [details] [diff] [review]
Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 4/6

Review of attachment 9011873 [details] [diff] [review]:
-----------------------------------------------------------------

This good looks good to me, but I'm not a necko peer. I cannot give you a r+.
Attachment #9011873 - Flags: review?(honzab.moz)
Attachment #9011873 - Flags: review?(amarchesini)
Attachment #9011873 - Flags: feedback+
Comment on attachment 9011873 [details] [diff] [review]
Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 4/6

Review of attachment 9011873 [details] [diff] [review]:
-----------------------------------------------------------------

not happy about splitting another http channel method, but there is no other way, r+

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +29,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsISupportsPrimitives.h"
>  #include "nsChannelClassifier.h"
>  #include "nsContentPolicyUtils.h"
> +#include "nsDOMNavigationTiming.h"

otherwise unified build breaks?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6389,5 @@
>  
> +    // PauseTask/DelayHttpChannel queuing
> +    if (!DelayHttpChannelQueue::AttemptQueueChannel(this)) {
> +        // If fuzzyfox is disabled; or adding to the queue failed, the channel must continue.
> +        AsyncOpenFinal(TimeStamp::Now());

note: this is fine because of: https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/netwerk/protocol/http/nsHttpChannel.cpp#6413-6417, bug 1271987
Attachment #9011873 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #166)
> Comment on attachment 9011873 [details] [diff] [review]
> Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests
> until FuzzyFox indicates they should fire 4/6
> 
> Review of attachment 9011873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> not happy about splitting another http channel method, but there is no other
> way, r+
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +29,5 @@
> >  #include "nsCOMPtr.h"
> >  #include "nsISupportsPrimitives.h"
> >  #include "nsChannelClassifier.h"
> >  #include "nsContentPolicyUtils.h"
> > +#include "nsDOMNavigationTiming.h"
> 
> otherwise unified build breaks?

Correct
I've read through the patches. lgtm overall. Faithfully implements the fuzzyfox logic.
Couple TODOs that Tom asked about:

> //TODO: David, we still sleep at this point, but I'm not sure why? Do we need to?
We do need to still sleep there.
The idea is that the fuzzyfox runnable always 'finishes out' a time epoch. Since we can't ensure we didn't go over budget, that function retroactively determines the last few time epochs.
Once its done doing that, it sleeps until the end of the new 'current' time epoch, which will always have >=0 time left.
We theorized some attacks being possible if the adversary can force 0-duration sleeps always by going over budget. Nothing concrete but we decided to be safe.

>//TODO: this should probably influence uptick/downtick stuff, and it doesn't
Can remove this TODO, or can implement uptick/downtick being affected by the retroactive epoch generation. We don't have any concrete reason this has to be done, but it seems like the 'right' answer from the theory perspective.
Basically, rewrite:
>	    while (over > nextDuration) {
>	      over -= nextDuration;
>	      nextDuration = PickDuration();
>	    }
as
>	    while (over > nextDuration) {
>	      over -= nextDuration;
>	      nextDuration = PickDuration();
>	      mTickType = mTickType == eUptick ? eDowntick : eUptick;
>	    }

Final note:
We may want to note somewhere that the fuzzyfox implementation is incomplete, and that delayhttpchannel is a template for how to 'fuzzy' any communications channel. That dream implementation would include things like websockets etc.
Flags: needinfo?(dkohlbre)
Okay, updated patch 1 - I think this is ready to review in-whole!
Attachment #9011870 - Attachment is obsolete: true
Attachment #9011870 - Flags: feedback?(dkohlbre)
Flags: needinfo?(nfroyd)
Flags: needinfo?(jdemooij)
Flags: needinfo?(amarchesini)
Attachment #9013332 - Flags: review?(nfroyd)
Attachment #9013332 - Attachment is patch: true
Comment on attachment 9013332 [details] [diff] [review]
Bug 1432429 Add FuzzyFox class and prefs 1/6

Review of attachment 9013332 [details] [diff] [review]:
-----------------------------------------------------------------

Bugzilla is yelling WINDOWS PATCH at me; please make sure this lands with Unix line endings, and not Windows ones.

::: layout/build/nsLayoutStatics.cpp
@@ +284,5 @@
>    }
>  
>    nsThreadManager::InitializeShutdownObserver();
>  
> +  mozilla::Fuzzyfox::Start();

I believe that putting this here means that Fuzzyfox will start for parent and content processes, but not other kinds of processes that don't spin up LayoutStatics (GPU / VR / Web Extension (?) / etc.).  This limitation is OK, I take it?

::: modules/libpref/init/all.js
@@ +1363,5 @@
>  // information so we can understand why it is needed.
>  pref("privacy.resistFingerprinting.autoDeclineNoUserInputCanvasPrompts", true);
> +
> +pref("privacy.fuzzyfox.enabled", false);
> +pref("privacy.fuzzyfox.clockgrainus", 100);

Like I said before: I don't understand putting the pref here if we're just setting it to what the default would be anyway.  I can see the argument for putting the enabled pref in here if some later patch #ifdefs MOZ_FUZZYFOX around it, but the clockgrain one seems not that useful to set in this file.

::: mozglue/misc/TimeStamp_windows.h
@@ +13,5 @@
>  
> +/**
> + * The [mt] unit:
> + *
> + * Many values are kept in ticks of the Performance Coutner x 1000,

Nit: "Performance Counter"

::: toolkit/components/fuzzyfox/Fuzzyfox.cpp
@@ +143,5 @@
> +  if (mStartTime == 0) {
> +    // This is the first time we are running afer enabling FuzzyFox. We need
> +    // to prevent time from going backwards, so for the first run we round the time up
> +    // to the next grain.
> +    mStartTime = CeilToGrain(ActualTime());

MOZ_ASSERT(mStartTime != 0); ?

@@ +151,5 @@
> +    mSanityCheck = true;
> +    LOG(Info, ("[FuzzyfoxEvent] PT(%p) Going to start Fuzzyfox, queuing up the job \n",
> +       this));
> +
> +    DISPATCH_AND_RETURN();

Instead of DISPATCH_AND_RETURN, WDYT about putting:

auto dispatcher = MakeScopeExit([]() {
  SystemGroup::Dispatch(TaskCategory::Other, r.forget();
});

at the top of this function and then just `return NS_OK;` wherever we return?  (Might have to futz with the lambda a bit to get it past the static analysis.)

@@ +172,5 @@
> +
> +    mSanityCheck = true;
> +    DISPATCH_AND_RETURN();
> +  }
> +  

Nit: trailing space.

::: toolkit/components/fuzzyfox/Fuzzyfox.h
@@ +15,5 @@
> +
> +
> +/*
> + * This topic publishes the new canonical time according to Fuzzyfox,
> + * in microseconds since the epoch. If code needs to know the current time, 

Nit: trailing space.

@@ +55,5 @@
> + * a concept called 'fuzzy time' that degrades all clocks (explicit and
> + * implicit). This is done through a combination of holding time constant
> + * during program execution and pausing program execution.
> + *
> + * @InProceedings{KS16, 

Nit: trailing space.
Attachment #9013332 - Flags: review?(nfroyd) → review+
Comment on attachment 9011871 [details] [diff] [review]
Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/6

Review of attachment 9011871 [details] [diff] [review]:
-----------------------------------------------------------------

I think making TimeStamp bigger is a blocking issue here.

::: mozglue/misc/TimeStamp.h
@@ +456,5 @@
>    }
>  
> +  bool UsedCanonicalNow() const { return mUsedCanonicalNow; }
> +  static MFBT_API bool GetFuzzyfoxEnabled();
> +  static MFBT_API void SetFuzzyfoxEnabled(bool aValue);

These are really strange APIs to have; should we make them only callable once or something, which some documentation about the same?

@@ +477,5 @@
>  
> +  static TimeStamp NowReally() { return NowReally(true); }
> +  static MFBT_API TimeStamp NowFuzzy(TimeStampValue aValue);
> +
> +  static MFBT_API void UpdateFuzzyTimeStamp(TimeStamp aValue);

Do we really want NowFuzzy/UpdateFuzzyTimeStamp to be public APIs?

@@ +619,4 @@
>  
>    static MFBT_API TimeStamp Now(bool aHighResolution);
>  
> +  static MFBT_API TimeStamp NowReally(bool aHighResolution);

Bikeshed time: I think this should be called NowUnfuzzed.

@@ +646,5 @@
>     */
>    TimeStampValue mValue;
> +
> +  // Indicates if the TimeStamp is a frozen timestamp from FuzzyFox or not
> +  bool mUsedCanonicalNow;

I would much prefer it if TimeStamp didn't get any bigger when Fuzzyfox was not enabled.  That would require a configure-flag based approach rather than a pref-based approach...though maybe we still want the pref?
Attachment #9011871 - Flags: review?(nfroyd)
Attachment #9011872 - Flags: review?(nfroyd) → review+
Comment on attachment 9012171 [details] [diff] [review]
Bug 1432429 Point the JS Runtimes at TimeStamp for the current time 5/6

Review of attachment 9012171 [details] [diff] [review]:
-----------------------------------------------------------------

I...guess this is OK?

::: mozglue/misc/TimeStamp.h
@@ +461,5 @@
>  
> +  static MFBT_API int64_t NowFuzzyTime();
> +  static MFBT_API void UpdateFuzzyTime(int64_t aValue);
> +
> +  static MFBT_API void UpdateFuzzyTimeStamp(TimeStamp aValue);

I feel like the UpdateFuzzy* functions should be one function, since I'm having a hard time thinking of times when you wouldn't want to update both things simultaneously.
Attachment #9012171 - Flags: review?(nfroyd) → review+
Reviews accomplished!
Flags: needinfo?(nfroyd)
Diff of the patches since the last r+: https://ritter.vg/misc/transient/fuzzy1-patchdiff.html
Attachment #9013332 - Attachment is obsolete: true
Attachment #9019725 - Flags: review?(nfroyd)
Attachment #9019725 - Attachment is patch: true
Diff of the patches since the last r+: https://ritter.vg/misc/transient/fuzzy2-patchdiff.html
Attachment #9011871 - Attachment is obsolete: true
Attachment #9019726 - Flags: review?(nfroyd)
Carrying forward r+ from the unnumbered comment after Comment 164
Attachment #9012171 - Attachment is obsolete: true
Attachment #9019729 - Flags: review+
Carrying forward r+ from chutten that he has given at least 6 times already, including comment 116, 136, 145, 162; plus several in mozreview and phabricator.
Attachment #9011875 - Attachment is obsolete: true
Attachment #9019730 - Flags: review+
Comment on attachment 9019725 [details] [diff] [review]
Bug 1432429 Add FuzzyFox class and prefs 1/6 r=baku,froydnj

Review of attachment 9019725 [details] [diff] [review]:
-----------------------------------------------------------------

Splinter is yelling WINDOWS PATCH at me; you should fix the line endings before you land this.
Attachment #9019725 - Flags: review?(nfroyd) → review+
Attachment #9019727 - Flags: review?(nfroyd) → review+
Comment on attachment 9019726 [details] [diff] [review]
Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/6 r=baku,froydnj

Review of attachment 9019726 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing up the Windows code.

I don't know that I'm super happy about the TimeStamp63Bit stuff...who knows if compilers will produce reasonable code for that.  But we can land it and see where we stand.

::: mozglue/misc/TimeStamp.cpp
@@ +147,5 @@
> +
> +MFBT_API void
> +TimeStamp::UpdateFuzzyTimeStamp(TimeStamp aValue)
> +{
> +  #ifdef XP_WIN

Why are the preprocessor directives here indented?  (Compare the directives in NowFuzzy, above.)

::: mozglue/misc/TimeStamp.h
@@ +48,5 @@
> +
> +  bool operator==(const TimeStamp63Bit aOther) const
> +  {
> +    return mUsedCanonicalNow == aOther.mUsedCanonicalNow &&
> +           mTimeStamp == aOther.mTimeStamp;

Do we think it's worth using an anonymous union so we can implement operator== as a straight 64-bit comparison, rather than having to split out the two fields?  (The other alternative is to just memcpy `this` and `aOther` to local uint64_t variables and compare those directly.)
Attachment #9019726 - Flags: review?(nfroyd) → review+
Carrrying forward r+ from Comment 180
Attachment #9019725 - Attachment is obsolete: true
Attachment #9020383 - Flags: review+
Carrying forward r+ from Comment 181, fixed nits.
Attachment #9019726 - Attachment is obsolete: true
Attachment #9020384 - Flags: review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebase patch 5. Carrying forward r+ from the unnumbered comment after Comment 164
Attachment #9019729 - Attachment is obsolete: true
Attachment #9020420 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920270da576f
Add FuzzyFox class and prefs. r=baku,froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00e84c0233b
Integrate FuzzyFox into the TimeStamp class. r=baku,froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b65a35258367
Clean up a few random areas of code to accomodate Fuzzyfox: Performance and RefreshDriver. r=froydnj,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d52af18f5c6
Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire. r=mayhemer,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/77626c8d6bee
Point the JS Runtimes at TimeStamp for the current time. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c3422995b8
Record the fuzzyfox pref in Telemetry. r=chutten
Keywords: checkin-needed
Depends on: 1502686
Blocks: 1503895
Depends on: 1505109
Depends on: 1505070
Depends on: 1506295
Depends on: 1505088
You need to log in before you can comment on or make changes to this bug.