1432429 - (fuzzyfox) [META] FuzzyFox meta-bug

Status: RESOLVED FIXED

(Core :: Security, enhancement, P1)

Description

[Andrea Marchesini [:baku]]

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 0 - --enable-fuzzyfox

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - PauseTask

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - TimeStamp::NowFuzzy

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - Performance API

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 4 - DelayHttpChannelQueue

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 5 - Worker fuzzy clock update

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 6 - Layout assertions

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 7 - JS fuzzy time update

Comment 9

[Andrea Marchesini [:baku]]

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.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 0 - --enable-fuzzyfox

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - FuzzyfoxRunnable

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - FuzzyfoxRunnable

This works cross platform thanks to std c11.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - FuzzyfoxRunnable

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.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8963661 [details] [diff] [review]
part 2 - TimeStamp::NowFuzzy

Luke, do you know who can help with the review of this patch?

Comment 15

[Luke Wagner [:luke]]

I think Waldo has done a lot with time.

Comment 16

[Mike Hommey [:glandium]]

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?

Comment 17

[David Kohlbrenner]

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)

Comment 18

[David Kohlbrenner]

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.

Comment 19

[Andrea Marchesini [:baku]]

> Didn't you want this to be linux only?

Not anymore. This works on any platform.

Comment 20

[Mike Hommey [:glandium]]

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?

Comment 21

[Mike Hommey [:glandium]]

(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.

Comment 22

[Andrea Marchesini [:baku]]

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?

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 7 - JS fuzzy time update

Comment 24

[Tom Ritter [:tjr]]

(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...

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 1 - Fuzzyfox

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 2 - TimeStamp::NowFuzzy

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 7 - JS fuzzy time update

Let's go step by step with the review. When the first to blocks are OK, I'll ask the review for the rest.

Comment 28

[Andrea Marchesini [:baku]]

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.

Comment 29

[Nathan Froyd [:froydnj]]

(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?

Comment 30

[Nathan Froyd [:froydnj]]

(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 31

[Nathan Froyd [:froydnj]]

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.

Comment 32

[Nathan Froyd [:froydnj]]

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.

Comment 33

[David Kohlbrenner]

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)

Comment 34

[Andrea Marchesini [:baku]]

> #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?

Comment 35

[Nathan Froyd [:froydnj]]

(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.

Comment 36

[Andrea Marchesini [:baku]]

> 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.

Comment 37

[Nathan Froyd [:froydnj]]

(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.

Comment 38

[Andrea Marchesini [:baku]]

Attachment: part 2 - TimeStamp::NowFuzzy

rebased

Comment 39

[Andrea Marchesini [:baku]]

Attachment: part 5 - Worker fuzzy clock update

rebased

Comment 40

[Andrea Marchesini [:baku]]

Attachment: part 7 - JS fuzzy time update

Comment 42

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a --enable-fuzzyfox switch 1/14

Review commit: https://reviewboard.mozilla.org/r/255432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255432/

Comment 43

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class 2/14

Review commit: https://reviewboard.mozilla.org/r/255434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255434/

Comment 44

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 TimeStamp::NowFuzzy 3/14

Review commit: https://reviewboard.mozilla.org/r/255436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255436/

Comment 45

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Performance API 4/14

Review commit: https://reviewboard.mozilla.org/r/255438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255438/

Comment 46

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 DelayHttpChannelQueue 5/14

Review commit: https://reviewboard.mozilla.org/r/255440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255440/

Comment 47

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Worker fuzzy clock update 6/14

Review commit: https://reviewboard.mozilla.org/r/255442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255442/

Comment 48

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Layout Assertions 7/14

Review commit: https://reviewboard.mozilla.org/r/255444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255444/

Comment 49

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 JS fuzzy time update 8/14

Review commit: https://reviewboard.mozilla.org/r/255446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255446/

Comment 50

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14

Review commit: https://reviewboard.mozilla.org/r/255448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255448/

Comment 51

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Improve the macros, logging, and comments in Fuzzyfox.cpp 10/14

Review commit: https://reviewboard.mozilla.org/r/255450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255450/

Comment 52

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Switch Fuzzyfox over to a pref 11/14

Review commit: https://reviewboard.mozilla.org/r/255452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255452/

Comment 53

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Adjust DelayQueueHTTPChannel based on whether or not Fuzzyfox is enabled 12/14

Review commit: https://reviewboard.mozilla.org/r/255454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255454/

Comment 54

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled 13/14

Review commit: https://reviewboard.mozilla.org/r/255456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255456/

Comment 55

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Refactor Timestamp to have a NowInternal function 14/14

Review commit: https://reviewboard.mozilla.org/r/255458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255458/

Comment 57

[Tom Ritter [:tjr]]

Comment on attachment 8990365 [details]
Bug 1432429 Create a --enable-fuzzyfox switch 1/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255432/diff/1-2/

Comment 58

[Tom Ritter [:tjr]]

Comment on attachment 8990366 [details]
Bug 1432429 Add FuzzyFox class 2/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255434/diff/1-2/

Comment 59

[Tom Ritter [:tjr]]

Comment on attachment 8990367 [details]
Bug 1432429 TimeStamp::NowFuzzy 3/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255436/diff/1-2/

Comment 60

[Tom Ritter [:tjr]]

Comment on attachment 8990368 [details]
Bug 1432429 Performance API 4/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255438/diff/1-2/

Comment 61

[Tom Ritter [:tjr]]

Comment on attachment 8990369 [details]
Bug 1432429 DelayHttpChannelQueue 5/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255440/diff/1-2/

Comment 62

[Tom Ritter [:tjr]]

Comment on attachment 8990370 [details]
Bug 1432429  Worker fuzzy clock update 6/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255442/diff/1-2/

Comment 63

[Tom Ritter [:tjr]]

Comment on attachment 8990371 [details]
Bug 1432429 Layout Assertions 7/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255444/diff/1-2/

Comment 64

[Tom Ritter [:tjr]]

Comment on attachment 8990372 [details]
Bug 1432429 JS fuzzy time update 8/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255446/diff/1-2/

Comment 65

[Tom Ritter [:tjr]]

Comment on attachment 8990373 [details]
Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255448/diff/1-2/

Comment 66

[Tom Ritter [:tjr]]

Comment on attachment 8990374 [details]
Bug 1432429 Improve the macros, logging, and comments in Fuzzyfox.cpp 10/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255450/diff/1-2/

Comment 67

[Tom Ritter [:tjr]]

Comment on attachment 8990375 [details]
Bug 1432429 Switch Fuzzyfox over to a pref 11/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255452/diff/1-2/

Comment 68

[Tom Ritter [:tjr]]

Comment on attachment 8990376 [details]
Bug 1432429 Adjust DelayQueueHTTPChannel based on whether or not Fuzzyfox is enabled 12/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255454/diff/1-2/

Comment 69

[Tom Ritter [:tjr]]

Comment on attachment 8990377 [details]
Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled 13/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255456/diff/1-2/

Comment 70

[Tom Ritter [:tjr]]

Comment on attachment 8990378 [details]
Bug 1432429 Refactor Timestamp to have a NowInternal function 14/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255458/diff/1-2/

Comment 72

[Tom Ritter [:tjr]]

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...

Comment 73

[Tom Ritter [:tjr]]

Comment on attachment 8990378 [details]
Bug 1432429 Refactor Timestamp to have a NowInternal function 14/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255458/diff/2-3/

Comment 74

[Tom Ritter [:tjr]]

(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 75

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a --enable-fuzzyfox switch 1/14

Review commit: https://reviewboard.mozilla.org/r/256300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256300/

Comment 76

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class 2/14

Review commit: https://reviewboard.mozilla.org/r/256302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256302/

Comment 77

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 TimeStamp::NowFuzzy 3/14

Review commit: https://reviewboard.mozilla.org/r/256304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256304/

Comment 78

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Performance API 4/14

Review commit: https://reviewboard.mozilla.org/r/256306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256306/

Comment 79

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 DelayHttpChannelQueue 5/14

Review commit: https://reviewboard.mozilla.org/r/256308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256308/

Comment 80

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Worker fuzzy clock update 6/14

Review commit: https://reviewboard.mozilla.org/r/256310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256310/

Comment 81

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Layout Assertions 7/14

Review commit: https://reviewboard.mozilla.org/r/256312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256312/

Comment 82

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 JS fuzzy time update 8/14

Review commit: https://reviewboard.mozilla.org/r/256314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256314/

Comment 83

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14

Review commit: https://reviewboard.mozilla.org/r/256316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256316/

Comment 84

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Improve the macros, logging, and comments in Fuzzyfox.cpp 10/14

Review commit: https://reviewboard.mozilla.org/r/256318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256318/

Comment 85

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Switch Fuzzyfox over to a pref 11/14

Review commit: https://reviewboard.mozilla.org/r/256320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256320/

Comment 86

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Adjust DelayQueueHTTPChannel based on whether or not Fuzzyfox is enabled 12/14

Review commit: https://reviewboard.mozilla.org/r/256322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256322/

Comment 87

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled 13/14

Review commit: https://reviewboard.mozilla.org/r/256324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256324/

Comment 89

[Tom Ritter [:tjr]]

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?

Comment 90

[Tom Ritter [:tjr]]

(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 91

[Andrea Marchesini [:baku]]

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.

Comment 92

[Andrea Marchesini [:baku]]

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?

Comment 93

[Andrea Marchesini [:baku]]

Comment on attachment 8991420 [details]
Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14

https://reviewboard.mozilla.org/r/256316/#review266286

Comment 94

[Andrea Marchesini [:baku]]

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 ..

Comment 95

[Andrea Marchesini [:baku]]

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

Comment 96

[Andrea Marchesini [:baku]]

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.

Comment 97

[Tom Ritter [:tjr]]

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

Comment 98

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Record the fuzzyfox pref in Telemetry 15/14

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

Review commit: https://reviewboard.mozilla.org/r/261248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261248/

Comment 99

[Tom Ritter [:tjr]]

Comment on attachment 8991412 [details]
Bug 1432429 Create a --enable-fuzzyfox switch 1/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256300/diff/1-2/

Comment 100

[Tom Ritter [:tjr]]

Comment on attachment 8991413 [details]
Bug 1432429 Add FuzzyFox class 2/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256302/diff/1-2/

Comment 101

[Tom Ritter [:tjr]]

Comment on attachment 8991414 [details]
Bug 1432429 TimeStamp::NowFuzzy 3/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256304/diff/1-2/

Comment 102

[Tom Ritter [:tjr]]

Comment on attachment 8991415 [details]
Bug 1432429 Performance API 4/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256306/diff/1-2/

Comment 103

[Tom Ritter [:tjr]]

Comment on attachment 8991416 [details]
Bug 1432429 DelayHttpChannelQueue 5/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256308/diff/1-2/

Comment 104

[Tom Ritter [:tjr]]

Comment on attachment 8991417 [details]
Bug 1432429  Worker fuzzy clock update 6/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256310/diff/1-2/

Comment 105

[Tom Ritter [:tjr]]

Comment on attachment 8991418 [details]
Bug 1432429 Layout Assertions 7/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256312/diff/1-2/

Comment 106

[Tom Ritter [:tjr]]

Comment on attachment 8991419 [details]
Bug 1432429 JS fuzzy time update 8/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256314/diff/1-2/

Comment 107

[Tom Ritter [:tjr]]

Comment on attachment 8991420 [details]
Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256316/diff/1-2/

Comment 108

[Tom Ritter [:tjr]]

Comment on attachment 8991421 [details]
Bug 1432429 Improve the macros, logging, and comments in Fuzzyfox.cpp 10/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256318/diff/1-2/

Comment 109

[Tom Ritter [:tjr]]

Comment on attachment 8991422 [details]
Bug 1432429 Switch Fuzzyfox over to a pref 11/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256320/diff/1-2/

Comment 110

[Tom Ritter [:tjr]]

Comment on attachment 8991423 [details]
Bug 1432429 Adjust DelayQueueHTTPChannel based on whether or not Fuzzyfox is enabled 12/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256322/diff/1-2/

Comment 111

[Tom Ritter [:tjr]]

Comment on attachment 8991424 [details]
Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled 13/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256324/diff/1-2/

Comment 112

[Tom Ritter [:tjr]]

Comment on attachment 8990378 [details]
Bug 1432429 Refactor Timestamp to have a NowInternal function 14/14

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255458/diff/3-4/

Comment 114

[Tom Ritter [:tjr]]

Attachment: fuzzyfox-data-collection-request.txt

Comment 115

[Tom Ritter [:tjr]]

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...

Comment 116

[Chris H-C :chutten]

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?

Comment 117

[Chris H-C :chutten]

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.

Comment 118

[Tom Ritter [:tjr]]

(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).

Comment 119

[Chris H-C :chutten]

Sounds fine by me.

Comment 120

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a --enable-fuzzyfox mozconfig switch 1/14 r=baku

This will eventually be removed, but we keep this commit to
preserve the original patch series.

Comment 121

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class 2/14 r=baku

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

Comment 122

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Integrate FuzzyFox into the TimeStamp class 3/14 r=baku

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

Depends on D3627

Comment 123

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Correct the Performance API 'NowUnclamped' to obtain a non-Fuzzed Timestamp 4/14 r=baku

MozReview-Commit-ID: 7nORNl67WKG

Depends on D3628

Comment 124

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create the DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 5/14 r=baku

Depends on D3629

Comment 125

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Update the clock in (Javascript) Workers 6/14 r=baku

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

Comment 126

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a method on TimeStamp to indiciate if the Time is Fuzzy 7/14 r=baku

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

Depends on D3634

Comment 127

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Update the current time on JS Runtimes, and use that time 8/14 r=baku

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

Comment 128

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Remove MOZ_FUZZYFOX ifdefs and build it unconditionally 9/14 r=baku

Depends on D3640

Comment 129

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Improve the macros, logging, and comments in Fuzzyfox.cpp 10/14 r=baku

MozReview-Commit-ID: 28abz1Lm0Qv

Depends on D3641

Comment 130

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Switch Fuzzyfox over to a pref 11/14 r=baku

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

Comment 131

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Adjust DelayQueueHTTPChannel based on whether or not Fuzzyfox is enabled 12/14 r=baku

MozReview-Commit-ID: FEEgXDRpvxG

Depends on D3646

Comment 132

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Tell the JS Runtime/Workers whether or not Fuzzyfox is enabled 13/14 r=baku

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

Comment 133

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Refactor Timestamp to have a NowInternal function 14/14 r=baku

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

Comment 134

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Record the fuzzyfox pref in Telemetry 15/14 r=baku

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

Comment 135

[Tom Ritter [:tjr]]

Attachment: all-fuzzyfox.patch

Attached is a patch that collapsed all the patches submitted for review so one can easily see the final changes.

Comment 136

[Chris H-C :chutten]

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.

Comment 137

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class and prefs 1/7 r=baku,froydnj

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.)

Comment 138

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/7 r=baku,froydnj

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

Comment 139

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Clean up a few random areas of code to accomodate Fuzzyfox: Performance and RefreshDriver 3/7 r=froydnj,baku

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

Comment 140

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 4/7 r=froydnj,baku

Depends on D5322

Comment 141

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Update the clock in (Javascript) Workers 5/7 r=baku

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

Comment 142

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Update the current time on JS Runtimes, and use that time 6/7 r=tcampbell

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

Comment 143

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Record the fuzzyfox pref in Telemetry 7/7 r=chutten

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 144

[Tom Ritter [:tjr]]

Try run with tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84172ef49bc71951b328a757aa807fa9da4ea746

Comment 145

[Chris H-C :chutten]

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.

Comment 146

[Nathan Froyd [:froydnj]]

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.

Comment 147

[Andrea Marchesini [:baku]]

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.

Comment 148

[Andrea Marchesini [:baku]]

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.

Comment 149

[Ted Campbell [:tcampbell]]

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.

Comment 150

[Andrea Marchesini [:baku]]

Comment on attachment 9007367 [details]
Bug 1432429 Update the clock in (Javascript) Workers 5/7 r=baku

Andrea Marchesini [:baku] has approved the revision.

Comment 151

[Andrea Marchesini [:baku]]

Comment on attachment 9007358 [details]
Bug 1432429 Add FuzzyFox class and prefs 1/7 r=baku,froydnj

Andrea Marchesini [:baku] has approved the revision.

Comment 152

[Tom Ritter [:tjr]]

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.

Comment 153

[Nathan Froyd [:froydnj]]

(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 ?

Comment 154

[Tom Ritter [:tjr]]

(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.

Comment 155

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class and prefs 1/6

Comment 156

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/6

Comment 157

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Clean up a few random areas of code to accomodate Fuzzyfox: Performance and RefreshDriver 3/6

Comment 158

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 4/6

Comment 159

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Point the JS Runtimes at TimeStamp for the current time

Comment 160

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Record the fuzzyfox pref in Telemetry 6/6

Comment 161

[Tom Ritter [:tjr]]

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 162

[Chris H-C :chutten]

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+

Comment 163

[Jan de Mooij [:jandem]]

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.

Comment 164

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Point the JS Runtimes at TimeStamp for the current time 5/6

Comment 165

[Andrea Marchesini [:baku]]

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+.

Comment 166

[Honza Bambas (:mayhemer)]

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

Comment 167

[Tom Ritter [:tjr]]

(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

Comment 168

[David Kohlbrenner]

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.

Comment 169

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class and prefs 1/6

Okay, updated patch 1 - I think this is ready to review in-whole!

Comment 170

[Nathan Froyd [:froydnj]]

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.

Comment 171

[Nathan Froyd [:froydnj]]

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?

Comment 172

[Nathan Froyd [:froydnj]]

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.

Comment 173

[Nathan Froyd [:froydnj]]

Reviews accomplished!

Comment 174

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class and prefs 1/6 r=baku,froydnj

Diff of the patches since the last r+: https://ritter.vg/misc/transient/fuzzy1-patchdiff.html

Comment 175

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/6 r=baku,froydnj

Diff of the patches since the last r+: https://ritter.vg/misc/transient/fuzzy2-patchdiff.html

Comment 176

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Clean up a few random areas of code to accomodate Fuzzyfox: Performance and RefreshDriver 3/6 r=froydnj,baku

Diff of the patches since the last r+: https://ritter.vg/misc/transient/fuzzy3-patchdiff.html

Comment 177

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Create a DelayHttpChannelQueue class to queue network requests until FuzzyFox indicates they should fire 4/6 r=mayhemer,baku

Carrying forward r+ from Comment 166

Comment 178

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Point the JS Runtimes at TimeStamp for the current time 5/6 r=jandem

Carrying forward r+ from the unnumbered comment after Comment 164

Comment 179

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Record the fuzzyfox pref in Telemetry 6/6 r=chutten

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.

Comment 180

[Nathan Froyd [:froydnj]]

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.

Comment 181

[Nathan Froyd [:froydnj]]

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.)

Comment 182

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Add FuzzyFox class and prefs 1/6 r=baku,froydnj

Carrrying forward r+ from Comment 180

Comment 183

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Integrate FuzzyFox into the TimeStamp class 2/6 r=baku,froydnj

Carrying forward r+ from Comment 181, fixed nits.

Comment 184

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 185

[Tom Ritter [:tjr]]

Attachment: Bug 1432429 Point the JS Runtimes at TimeStamp for the current time 5/6 r=jandem

Rebase patch 5. Carrying forward r+ from the unnumbered comment after Comment 164

Comment 186

[Pulsebot]

Pushed by [e-mail]:
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

Comment 187

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/920270da576f
https://hg.mozilla.org/mozilla-central/rev/b00e84c0233b
https://hg.mozilla.org/mozilla-central/rev/b65a35258367
https://hg.mozilla.org/mozilla-central/rev/3d52af18f5c6
https://hg.mozilla.org/mozilla-central/rev/77626c8d6bee
https://hg.mozilla.org/mozilla-central/rev/19c3422995b8