993906 - Package and send appropriate data with remotely hosted links request

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Bug 986521 creates the request to the remotely hosted links, and we'll want to send additional data in that request to provide metrics on impressions, clicks, etc with this bug.

Comment 1

[Olivier Yiptong [:oyiptong]]

A data packet is sent on two events:

1. when the newtab page is shown
2. upon directory link click

The data packet is only sent in 1. when there are any directory links shown.

Bug 993901 is also concerned about if directory links are shown and could perhaps re-use some code.

Comment 2

[Olivier Yiptong [:oyiptong]]

This bug description has changed. This bug is about sending a data packet on an event unrelated to those described above. Here is the task it describes:

Send a data payload when a fresh batch of links is requested from the server.
It will contain:

1. an identifier for the user, set as a cookie
2. the number of directory link slots available on the newtab page

clarkbw: is the data point described in #2 conformant to our terms of service?

Comment 3

[Ed Lee :Mardak]

clarkbw, to be clear, this bug is related to the data we sent with the request for directory links. I imagine this to include locale and potentially interests down the line.

This is separate from bug 975235 that will send data to count impressions and clicks.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Olivier Yiptong [:oyiptong] from comment #2)
> This bug description has changed. This bug is about sending a data packet on
> an event unrelated to those described above. Here is the task it describes:
> 
> Send a data payload when a fresh batch of links is requested from the server.
> It will contain:
> 
> 1. an identifier for the user, set as a cookie
> 2. the number of directory link slots available on the newtab page
  3. locale
 
> clarkbw: is the data point described in #2 conformant to our terms of
> service?

I'll have to check with Geof on #2 as I'm not sure.  We should be including the locale (#3) so we can match it to the language we offer.

Comment 5

[Olivier Yiptong [:oyiptong]]

Thanks for your comments, Bryan.

That means that the client implementation needs to accept an HTTP response with status code 204, in the case that there are no links for the requested locale.

204 implies that the request was successful, but that there is no content. The response will not have a body.

Comment 6

[Olivier Yiptong [:oyiptong]]

I propose the locale be sent via the url.

e.g. https://tiles.data.mozilla.com/v1/links/newtab/en-US

This makes the client code much simpler.

The number of open directory link slots seem to make sense as a header.
As a query parameter it does not.

It looks like this can be a GET, unless i'm missing something.

Comment 7

[Olivier Yiptong [:oyiptong]]

After a conversation with emtwo and maksik, we have agreed that this is a POST, and the locale will be sent in a payload.

The decision to put the locale in a payload is because

1. a user may request multiple locales
2. we may want to return additional localized links based on geo-ip
3. we may send additional data in the payload for reporting

The request should have a header of Content-Type: application/json, and the body would look like:

{"locale": "en-US"}

This affects bug 986521 as well

Comment 8

[Ed Lee :Mardak]

(In reply to Olivier Yiptong [:oyiptong] from comment #7)
> 1. a user may request multiple locales
> {"locale": "en-US"}
How would we include multiple locales and when would we do that? Perhaps by detecting the languages from the pages loaded?

Comment 9

[Olivier Yiptong [:oyiptong]]

getLocale() currently fetches from firefox prefs.

To include multiple locales, instead of a string value to the locale key, an array of locales could be provided.

we could obtain page languages as well, and together with geoip decide to return a certain set of links.

Comment 10

[Ed Lee :Mardak]

Right, I was just thinking if the POST data contained "locale": <string>, it would be trickier to change the format to have multiple locales. Perhaps there's a single string for "browserLocale" and an optional array of "extraLocales" that might be detected from page languages or other sources.

Comment 11

[maxim zhilyaev]

Attachment: v1: logic to fetch external directory links

Comment 12

[maxim zhilyaev]

Attachment: v2

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8421051 [details] [diff] [review]
v2

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

Since links aren't always fetched when reportShownCount is called, and the sites on newtab can change over time, there will be gaps of time where the directory counts are different from those in the directoryCount objects sent with requests.  Is that OK?  Should counts be accumulated between requests?

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +196,5 @@
>      try {
>        xmlHttp.open('POST', uri);
> +      xmlHttp.send(JSON.stringify({
> +        locale: this.locale,
> +        directoryCount: this._directoryCount,

I don't know your server, but you might want:

this._directoryCount || {}

since fetch is called by init and may happen before reportShownCount is ever called.  Or just initialize _directoryCount somewhere.

Comment 14

[maxim zhilyaev]

(In reply to Drew Willcoxon :adw from comment #13)
> Comment on attachment 8421051 [details] [diff] [review]
> v2
> 
> Review of attachment 8421051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since links aren't always fetched when reportShownCount is called, and the
> sites on newtab can change over time, there will be gaps of time where the
> directory counts are different from those in the directoryCount objects sent
> with requests.  Is that OK?  Should counts be accumulated between requests?
> 
> ::: toolkit/modules/DirectoryLinksProvider.jsm
> @@ +196,5 @@
> >      try {
> >        xmlHttp.open('POST', uri);
> > +      xmlHttp.send(JSON.stringify({
> > +        locale: this.locale,
> > +        directoryCount: this._directoryCount,
> 
> I don't know your server, but you might want:
> 
> this._directoryCount || {}
> 
> since fetch is called by init and may happen before reportShownCount is ever
> called.  Or just initialize _directoryCount somewhere.

This was actually done on purpose, so that if this._directoryCount is null, directoryCount is property is stripped from JSON payload to the backend server.

Comment 15

[Marco Mucci [:MarcoM]]

Removed from iteration.  Will be tracked the same way we track "non-regular" contributions from the community.

Comment 16

[maxim zhilyaev]

Attachment: v.2.1 rebased onto bug993901

Comment 17

[Ed Lee :Mardak]

Comment on attachment 8426548 [details] [diff] [review]
v.2.1 rebased onto bug993901

>diff --git a/toolkit/modules/DirectoryLinksProvider.jsm b/toolkit/modules/DirectoryLinksProvider.jsm
>   reportShownCount: function DirectoryLinksProvider_reportShownCount(directoryCount) {
>+    this._directoryCount = directoryCount;
This is causing a bc1 debug leak failure..
https://tbpl.mozilla.org/?tree=Try&rev=fc574de062dc

It seems to get fixed with this change instead:
+    this._directoryCount = JSON.parse(JSON.stringify(directoryCount));
https://tbpl.mozilla.org/?tree=Try&rev=51e03099d523

The directoryCount object is created with {} with keys/values of just the types and numbers. Perhaps somewhere the Object prototype is changed from newtab.js somewhere?

Comment 18

[maxim zhilyaev]

Attachment: v3. Make a deep copy of directoryCount

Comment 19

[Ed Lee :Mardak]

Comment on attachment 8428411 [details] [diff] [review]
v3.  Make a deep copy of directoryCount

>+++ b/toolkit/modules/DirectoryLinksProvider.jsm
>   reportShownCount: function DirectoryLinksProvider_reportShownCount(directoryCount) {
>+    // make a deep copy of directoryCount to avoid a leak
>+    this._directoryCount = JSON.parse(JSON.stringify(directoryCount));
maksik, did you look into why this was necessary?

Comment 20

[Drew Willcoxon :adw]

Comment on attachment 8428411 [details] [diff] [review]
v3.  Make a deep copy of directoryCount

Clearing review request since I understand another patch is coming.

Comment 21

[Drew Willcoxon :adw]

Maxim explained that he was thinking of adding a DirectoryLinksProvider method that returned the directoryLinks object instead of having the consumer pass in the object, but then there'd need to be another public method that triggered the fetch, since reportShownCount does that now.  I didn't really like that added complexity, so I think it's fine to clone the passed-in object if it fixes the leak.  JSON.parse(JSON.stringify()) kind of sucks, but whatever.

But I think the leak might have something to do with xpconnect or compartment boundaries, so I pushed a few things to try, and I'd like to see if they work before r+'ing the JSON patch here.  If they do work, I'm not sure it proves my guess was right, though.

https://tbpl.mozilla.org/?tree=Try&rev=024ba31e5197
this._directoryCount = Cu.cloneInto(directoryCount, this);

https://tbpl.mozilla.org/?tree=Try&rev=2a803867988f
this._directoryCount = Cu.cloneInto(directoryCount, this,
                                    { cloneFunctions: true });

https://tbpl.mozilla.org/?tree=Try&rev=86d197ebe144
this._directoryCount = directoryCount;
Cu.makeObjectPropsNormal(this._directoryCount);

Comment 22

[Ed Lee :Mardak]

I haven't seen cloneInto before, but searching in mxr, the pattern seems to be cloneInto(source, {}) as opposed to "this" ? The tests seem to be passing with cloneInto, so perhaps we'll just use that instead of parse/stringify.

Comment 23

[Drew Willcoxon :adw]

Comment on attachment 8428411 [details] [diff] [review]
v3.  Make a deep copy of directoryCount

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

The second arg is used to root the new object as far as I can tell, so it seems like `this` and {} ought to be equivalent in this case.  But let's go with cloneInto, using either {} or this, instead of JSON.

Comment 24

[Drew Willcoxon :adw]

Er, I meant to cite my source: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp?rev=5021d1337fa9#3579

Comment 25

[maxim zhilyaev]

Attachment: v3.1 Use Cu.cloneInto for object copy

Comment 26

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/84fbe9c9231c

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84fbe9c9231c