1454100 - Cookies like to move around

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement, P2)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1454100 - Move cookies around every X seconds

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

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1454100 - Add some telemetry for time spent moving cookies

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

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968161 [details]
Bug 1454100 - Move cookies around every X seconds

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

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

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

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968161 [details]
Bug 1454100 - Move cookies around every X seconds

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

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

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

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8968161 [details]
Bug 1454100 - Move cookies around every X seconds

https://reviewboard.mozilla.org/r/236828/#review244158

::: netwerk/cookie/CookieServiceChild.cpp:692
(Diff revision 3)
> +    return NS_OK;
> +  }
>  
>    nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
>    if (prefBranch)
>      PrefChanged(prefBranch);

would be a bit nicer to have the code like:

if (!strcmp("pref-change")) {
  ...
} else if (!strcmp("xpcom-shut")) {
  ...
} else {
  MOZ_ASSERT(FALSE);
}

but up to you

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

https://reviewboard.mozilla.org/r/236830/#review244160

::: netwerk/cookie/CookieServiceChild.cpp:119
(Diff revision 3)
>  }
>  
>  void
>  CookieServiceChild::MoveCookies()
>  {
> +  TimeStamp before = TimeStamp::Now();

I think `start` is a better name

::: netwerk/cookie/CookieServiceChild.cpp:142
(Diff revision 3)
>        newCookiesList.AppendElement(newCookie);
>      }
>      cookiesList->SwapElements(newCookiesList);
>    }
> +
> +  Telemetry::Accumulate(Telemetry::COOKIE_TIME_MOVING_MS,

There is AccumulateTimeDelta which is more suitable.

::: toolkit/components/telemetry/Histograms.json:10150
(Diff revision 3)
> +    "record_in_processes": ["content"],
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "bug_numbers": [1454100],
> +    "expires_in_version": "64",
> +    "kind": "exponential",
> +    "high": 30000,

I think this should be lower, way lower.  what is in your opinion a time we consider as "if it's more than this, we have a problem"?  I think 3 seconds is already a barrier.  Other reason is to have a better granularity at the lower end of the probe, IMO.

::: toolkit/components/telemetry/Histograms.json:10152
(Diff revision 3)
> +    "bug_numbers": [1454100],
> +    "expires_in_version": "64",
> +    "kind": "exponential",
> +    "high": 30000,
> +    "n_buckets": 20,
> +    "description": "Time spent to perform one cookie move operation - moves cookies in memory from one location to another (ms)"

"Time spent to perform all cookies move operation" ?

Comment 9

[(no longer active)]

Comment on attachment 8968161 [details]
Bug 1454100 - Move cookies around every X seconds

https://reviewboard.mozilla.org/r/236828/#review244324

r=me with the comments below fixed.  The second comment is especially important.  Thanks!

::: netwerk/cookie/CookieServiceChild.cpp:110
(Diff revision 3)
>    }
> +
> +  nsCOMPtr<nsIObserverService> observerService
> +    = mozilla::services::GetObserverService();
> +  if (observerService) {
> +    observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, true);

Nit: since you're already handling xpcom-shutdown, it's a bit more efficient to pass false for the third argument and remove the observer explicitly when shutting down...

::: netwerk/cookie/CookieServiceChild.cpp:144
(Diff revision 3)
> +
> +NS_IMETHODIMP
> +CookieServiceChild::Notify(nsITimer *aTimer)
> +{
> +  if (aTimer == mCookieTimer) {
> +    MoveCookies();

Using a timer will mean that the timer callback can run at times that we have important work to do on the main thread.  Since MoveCookies() can potentially take a long time to run, this has a chance of blowing into the frame budget when doing work that is sensitive to performance.

Instead, please use NS_IdleDispatchToCurrentThread() <https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/xpcom/threads/nsThreadUtils.h#154> to dispatch a runnable to the idle queue which is responsible to call MoveCookies().  You need to pass a timeout argument here to ensure that if the low-priority idle queue doesn't get a chance to run within a short period of time, this task will still get scheduled on the normal priority message queue of the main thread.

You should decide on a timeout value to pass here.  I suggest passing some value that is sufficiently large to allow the event to be dispatched with low priority in most cases but is lower than the value of the pref.  I would go with something like min(1, gMoveCookiesIntervalSeconds) * 1000.

Comment 10

[(no longer active)]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

https://reviewboard.mozilla.org/r/236830/#review244328

Comment 11

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Ehsan Akhgari from comment #9)
> ::: netwerk/cookie/CookieServiceChild.cpp:144
> (Diff revision 3)
> > +
> > +NS_IMETHODIMP
> > +CookieServiceChild::Notify(nsITimer *aTimer)
> > +{
> > +  if (aTimer == mCookieTimer) {
> > +    MoveCookies();
> 
> Using a timer will mean that the timer callback can run at times that we
> have important work to do on the main thread.  Since MoveCookies() can
> potentially take a long time to run, this has a chance of blowing into the
> frame budget when doing work that is sensitive to performance.
> 
> Instead, please use NS_IdleDispatchToCurrentThread()
> <https://searchfox.org/mozilla-central/rev/
> 11a2ae294f50049e12515b5821f5a396d951aacb/xpcom/threads/nsThreadUtils.h#154>
> to dispatch a runnable to the idle queue which is responsible to call
> MoveCookies().

From what I can tell a timer with nsITimer::TYPE_REPEATING_SLACK_LOW_PRIORITY already has a lower priority than "idle callbacks"

https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/xpcom/threads/nsITimer.idl#120-125
Would NS_IdleDispatchToCurrentThread be different from this?

> You need to pass a timeout argument here to ensure that if
> the low-priority idle queue doesn't get a chance to run within a short
> period of time, this task will still get scheduled on the normal priority
> message queue of the main thread.
> 
> You should decide on a timeout value to pass here.  I suggest passing some
> value that is sufficiently large to allow the event to be dispatched with
> low priority in most cases but is lower than the value of the pref.  I would
> go with something like min(1, gMoveCookiesIntervalSeconds) * 1000.

So, I should still use a timer, so that we call MoveCookies() at a certain interval, but then Notify should call NS_IdleDispatchToCurrentThread instead of calling MoveCookies directly?
Otherwise, when idle we would be moving the cookies all the time, which isn't exactly what we want.

Comment 12

[(no longer active)]

(In reply to Valentin Gosu [:valentin] from comment #11)
> (In reply to :Ehsan Akhgari from comment #9)
> > ::: netwerk/cookie/CookieServiceChild.cpp:144
> > (Diff revision 3)
> > > +
> > > +NS_IMETHODIMP
> > > +CookieServiceChild::Notify(nsITimer *aTimer)
> > > +{
> > > +  if (aTimer == mCookieTimer) {
> > > +    MoveCookies();
> > 
> > Using a timer will mean that the timer callback can run at times that we
> > have important work to do on the main thread.  Since MoveCookies() can
> > potentially take a long time to run, this has a chance of blowing into the
> > frame budget when doing work that is sensitive to performance.
> > 
> > Instead, please use NS_IdleDispatchToCurrentThread()
> > <https://searchfox.org/mozilla-central/rev/
> > 11a2ae294f50049e12515b5821f5a396d951aacb/xpcom/threads/nsThreadUtils.h#154>
> > to dispatch a runnable to the idle queue which is responsible to call
> > MoveCookies().
> 
> From what I can tell a timer with
> nsITimer::TYPE_REPEATING_SLACK_LOW_PRIORITY already has a lower priority
> than "idle callbacks"
> 
> https://searchfox.org/mozilla-central/rev/
> 8f06c1b9a080b84435a2906e420fe102e1ed780b/xpcom/threads/nsITimer.idl#120-125
> Would NS_IdleDispatchToCurrentThread be different from this?

Hmm, you're right, I didn't pay attention to that.  This means that idle events can run at times when we would have liked to dispatch these types of timers (so in a way, they're higher priority than these timers), so I suppose using these timers as your current patch is doing should be OK.  Please disregard my second comment.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968161 [details]
Bug 1454100 - Move cookies around every X seconds

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

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

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

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

1. What questions will you answer with this data?

How long the cookie move operation takes on the average user's machine, and if it is likely for the operation to cause usability issues (hangs)

2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? Some example responses:

Good user experience requires that we don't cause hangs :)

3. What alternative methods did you consider to answer these questions? Why were they not sufficient?

Local testing, but this is unlikely to reliably answer the questions.
Another way would be to only rely on the BHR. However, we would like to know how long this operation takes even if it doesn't cause hangs.

4. Can current instrumentation answer these questions?

No.

5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the found on the Mozilla wiki.

I categorize the new probe as Category 1 “Technical data”.

6. How long will this data be collected? Choose one of the following:

I want this data to be collected for 6 months initially (potentially renewable).

7. What populations will you measure?

Everyone, in all channels.

8. If this data collection is default on, what is the opt-out mechanism for users?

Nope

9. Please provide a general description of how you will analyze this data.

I'll use telemetry.mozilla.org

10. Where do you intend to share the results of your analysis?

The results are public on t.m.o

Comment 16

[François Marier [:francois]]

Comment on attachment 8968162 [details]
Bug 1454100 - Add some telemetry for time spent moving cookies

https://reviewboard.mozilla.org/r/236830/#review246574

(In reply to Valentin Gosu [:valentin] from comment #15)

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

Yes, in Histograms.json.

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

Yes, telemetry setting.

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

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

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

Default OFF on release, default ON on pre-release.

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

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

Yes.

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

No, telemetry alerts are enough.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ac5ad8824e4
Move cookies around every X seconds r=Ehsan,mayhemer
https://hg.mozilla.org/integration/autoland/rev/83a92ca59b1e
Add some telemetry for time spent moving cookies r=Ehsan,francois,mayhemer

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2ac5ad8824e4
https://hg.mozilla.org/mozilla-central/rev/83a92ca59b1e