Closed Bug 1054202 Opened 6 years ago Closed 6 years ago

Disable enhanced tiles ping, directory tiles fetch, firstrun intro for talos

Categories

(Testing :: Talos, defect)

defect
Not set
Points:
2

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34
Iteration:
34.3

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

From bug 1042214, the patch adds

+++ b/testing/profiles/prefs_general.js
+// Don't fetch or send directory tiles data from real servers
+user_pref("browser.newtabpage.directory.source", 'data:application/json,{"testing":1}');
+user_pref("browser.newtabpage.directory.ping", "");

We'll want the equivalent for talos to not hit "FATAL ERROR: Non-local network connections are disabled and a connection attempt to tiles.up.mozillalabs.com (75.101.161.143) was made."
Flags: firefox-backlog+
Attached patch v1 (obsolete) — Splinter Review
Attachment #8473580 - Flags: review?(jmaher)
What are the implications of this modification? Specifically, does it prevent DT from showing up during TART, therefore resulting in "better" performance numbers than what users might experience?
The v1 patch turns off directory tiles by setting the links data to {}. We could package it with the current set of directory links so there shouldn't be performance deltas from what's currently running.
Attached patch wip v1 (obsolete) — Splinter Review
Is there a good way to read out the webserver when setting preferences? Otherwise we could just convert the file into a data uri and set that as the pref.
Attachment #8473755 - Flags: feedback?(jmaher)
I find it hard to interpret your reply. DT have different configurations on different branches.

My question was, per branch, if this patch makes it behave differently between talos and users' systems, and if yes, differently how?
(In reply to Avi Halachmi (:avih) from comment #5)
> DT have different configurations on different branches.
Oh, right. These talos changes would affect tests running for all of nightly/aurora/beta/release.

Currently release is empty, beta is supposed to be empty, aurora has plain directory tiles, nightly has enhanced tiles. Soon nightly will have remotely hosted tiles (which led to this bug).

I would assume in order for bug 1042214 to land, we need to fix this otherwise svgr will turn red for "FATAL ERROR: Non-local network connections are disabled and a connection attempt to tiles.up.mozillalabs.com (75.101.161.143) was made."
Thanks for the good info. This question still stands, however:

(In reply to Avi Halachmi (:avih) from comment #5)
> ...
> My question was, per branch, if this patch makes it behave differently
> between talos and users' systems, and if yes, differently how?
(In reply to Avi Halachmi (:avih) from comment #5)
> My question was, per branch, if this patch makes it behave differently
> between talos and users' systems, and if yes, differently how?
Today, release/beta/aurora/nightly behave slightly differently per comment 6; so setting one pref as the v1 patch does would cause the test to run differently than what release/beta/aurora/nightly users would have seen.

With the v1 patch of turning off directory tiles:
- release would be unchanged
- beta would be as desired (but run into crash currently debugging bug 1039881)
- aurora would see an inaccurate perf boost because it would no longer show tiles
- nightly would also see perf improvement (and not see enhanced tiles)
Great info again, thanks.

(In reply to Ed Lee :Mardak from comment #8)
> With the v1 patch of turning off directory tiles:

Patch v1 does seem to turn them off, but the patch "v1 wip" (comment 4) seems to change the DT source to the talos web server instead of a mozilla server which as far as I can understand, _will_ show directory tiles also inside talos.

So what's the goal here? always disabling DT on talos? or moving the tiles source to the talos server?

I'd argue that we want the talos config to be as similar to users' config per branch. Personally, I'd consider the tiles source at the talos server similar enough.

Not sure what "remote tiles" are exactly (specifically - if "Remote tiles" use different remote server/mechanism than "normal" tiles), but if we could also use the talos server for this - only for versions which have it enabled, then it could be a good solution IMO.
(In reply to Avi Halachmi (:avih) from comment #9)
> So what's the goal here? always disabling DT on talos? or moving the tiles
> source to the talos server?
Yes, the two patches do very different things. Off vs on. Where off is simple and "on" might require some interesting hosting or "on" could just set the pref to a really long data URI (should be pretty simple to do).

> I'd argue that we want the talos config to be as similar to users' config
> per branch.
I can agree with that, although in this situation (see details in comment 8), beta/release will only be blank for one more full cycle, so going forwards, they'll have directory tiles turned on. Getting the right per-branch detecting and setting will make things more complicated, so if it's okay, the simple thing is to have talos run with tiles on with enhanced data from nightly (which will be served remotely when bug 1042876 lands this week).
(In reply to Ed Lee :Mardak from comment #10)
> Yes, the two patches do very different things. Off vs on. Where off is
> simple and "on" might require some interesting hosting or "on" could just
> set the pref to a really long data URI (should be pretty simple to do).
Where I'm guessing on the difficulty of the hosting stuff as I have very little knowledge of patching talos.

jmaher, you might have a better idea of what would be involved in locally hosting the data file and setting the appropriate uri as a Firefox preference? (see patches for my initial attempts).
Flags: needinfo?(jmaher)
Note the talos revision used for each branch is pinned. ie: trunk/aurora/beta don't have to (and aren't currently) all running the same talos rev. We occasionally do update the talos version being used on branches, but not often. Therefore you don't need to worry about branches, unless the patch this is blocking is going to be uplifted to the release branches.
Thanks for the info. So for bug 1042214 to land and avoid "FATAL ERROR: Non-local network connections are disabled..." for talos, we'll want talos-for-nightly to override a pref. When it moves to aurora/beta, how do we make sure talos-for-aurora/beta also overrides the pref?
The revision is pinned in a manifest in the tree, so the rev will carry from trunk to aurora as part of the merge:
http://mxr.mozilla.org/mozilla-central/source/testing/talos/talos.json#8
(In reply to Ed Lee :Mardak from comment #10)
> > I'd argue that we want the talos config to be as similar to users' config
> > per branch.
> I can agree with that, although in this situation (see details in comment
> 8), beta/release will only be blank for one more full cycle, so going
> forwards, they'll have directory tiles turned on.

I agree. Let's start with a patch to turn them on within talos first.

Would static hosting be enough for this? or does it communicate via some handshaking which needs server side code?

Also, I'm a bit confused with the terminology. How many types of Directory tiles do we have (in the near future)? Are there "normal" Directory tiles? and then what are "enhanced" tiles? "remote" tiles?
Comment on attachment 8473755 [details] [diff] [review]
wip v1

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

this approach is fine but a couple issues:
1) port 15707 is only used on the local server, in production we run on apache and the default port.  Here is an example of using the current webserver: http://hg.mozilla.org/build/talos/file/tip/talos/PerfConfigurator.py#l765.  This would need a few changes to do a similar thing for preferences.
2) we have real url's for the directory tiles.  Does this not touch the internet?  When we landed session restore with real urls (even though we never loaded the tabs) we still had external network access.
Attachment #8473755 - Flags: feedback?(jmaher) → feedback-
Comment on attachment 8473580 [details] [diff] [review]
v1

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

I like this except for the fact that it will affect tart (as :avih pointed out).
Attachment #8473580 - Flags: review?(jmaher) → review+
looking into this more, we can modify the preferences here:
http://hg.mozilla.org/build/talos/file/tip/talos/ffsetup.py#l225

I would think we could call interpolatePath():
http://hg.mozilla.org/build/talos/file/tip/talos/utils.py#l120

and when calling that we could add in the webserver.  In the ffsetup.py code we could do:
for pref in prefs:
    value = utils.interpolatePath(prefs[perf], webserver=webserver)
    user_js_file.write(self.PrefString(pref, value, '\n'))

and then in utils.py we could modify interpolatePath to access a webserver and translate anything that has ${webserver} to be the value provided.
Flags: needinfo?(jmaher)
Depends on: 1054996
I filed bug 1054996 to fix talos to support this, then the patch for solving directory tiles will be quick and easy!
(In reply to Ed Morley [:edmorley] from comment #14)
> The revision is pinned in a manifest in the tree, so the rev will carry from
> trunk to aurora as part of the merge:
> http://mxr.mozilla.org/mozilla-central/source/testing/talos/talos.json#8
Ahhh neat. So I should update the m-c patch for bug 1042214 with the appropriate revision that we land for this bug?
We typically file "update talos revision to X" bugs, since it will likely deploy several bugs.
Attached patch v2 (obsolete) — Splinter Review
Patches on top of assumed bug 1054996 implementation.
Attachment #8473755 - Attachment is obsolete: true
Attachment #8474642 - Flags: review?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #16)
> 2) we have real url's for the directory tiles.  Does this not touch the
> internet?  When we landed session restore with real urls (even though we
> never loaded the tabs) we still had external network access.
The directory links point to live sites, but unless the tests click on tiles, there shouldn't be any outbound connections. (No thumbnail generated because it has a data uri image.) The session restore might have triggered connections because of the background preloading of newtab followed by background thumbnailing of the shown tiles.
Comment on attachment 8474642 [details] [diff] [review]
v2

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

this looks great, we will see who implements the ${webserver} replacement for preferences first :)
Attachment #8474642 - Flags: review?(jmaher) → review+
Comment on attachment 8474642 [details] [diff] [review]
v2

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

::: talos/PerfConfigurator.py
@@ +280,4 @@
>          'toolkit.telemetry.notifiedOptOut': 999,
>          'dom.send_after_paint_to_content': True,
>          'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer': True,
> +        'browser.newtabpage.directory.source': 'http://${webserver}/directoryLinks.json',

Wouldn't this be better as '${webserver}/directoryLinks.json' (i.e. without the protocol)?
Whiteboard: [qa?]
Summary: Disable enhanced tiles ping and directory tiles fetch for talos → Disable enhanced tiles ping, directory tiles fetch, firstrun intro for talos
Attached patch for checkinSplinter Review
(In reply to Avi Halachmi (:avih) from comment #25)
> Wouldn't this be better as '${webserver}/directoryLinks.json'
Done. The plain webserver value is 'localhost' but I made it detect if http:// needs to be prepended in bug 1054996.
Attachment #8473580 - Attachment is obsolete: true
Attachment #8474642 - Attachment is obsolete: true
http://hg.mozilla.org/build/talos/rev/49b74c08dad4
Assignee: nobody → edilee
Iteration: --- → 34.3
Points: --- → 2
Target Milestone: --- → mozilla34
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Ed Lee :Mardak from comment #6)
> Currently release is empty, beta is supposed to be empty, aurora has plain
> directory tiles, nightly has enhanced tiles. Soon nightly will have remotely
> hosted tiles (which led to this bug).

Could you describe each of those in a sentence please? (plain/enhanced/remotely-hosted).

Does the patch imitate the "remotely hosted" variant, just with the talos web server instead of a global mozilla server?
Flags: needinfo?(edilee)
I think something is still not quite right.

I'm running talos TART locally on latest nightly, and at the newtab tests I see only 1 tile - the rest are empty dashed frames.

Is it expected? could you please describe in words how should the newtab page look with this patch within talos/TART?
Plain directory tiles show an image/logo instead of empty tiles.

Enhanced tiles allow a image that changes to a logo on rollover.

Remotely hosted appears the same as enhanced except the data file is no longer bundled with Firefox, so fetched from a server.

Latest nightly shouldn't have bug 1042214 or bug 1042876 yet (those landed on fx-team). However, the latest talos would set the pref anyway to point to the localhost served directoryLinks.json file, so it should be showing 9 tiles of content.

(In reply to Avi Halachmi (:avih) from comment #29)
> at the newtab tests I see only 1 tile - the rest are empty dashed frames.
Can you check what browser.newtabpage.* preferences are? In particular browser.newtabpage.directory.source

And if that has a localhost file, what if you try loading that page in the browser?
Flags: needinfo?(edilee)
(In reply to Ed Lee :Mardak from comment #30)
> Plain directory tiles show an image/logo instead of empty tiles.
> 
> Enhanced tiles allow a image that changes to a logo on rollover.
> 
> Remotely hosted appears the same as enhanced except the data file is no
> longer bundled with Firefox, so fetched from a server.
> 
> Latest nightly shouldn't have bug 1042214 or bug 1042876 yet (those landed
> on fx-team). However, the latest talos would set the pref anyway to point to
> the localhost served directoryLinks.json file, so it should be showing 9
> tiles of content.

Ah. Thanks.

> Can you check what browser.newtabpage.* preferences are? In particular
> browser.newtabpage.directory.source
> 
> And if that has a localhost file, what if you try loading that page in the
> browser?

Really hard to check.

Talos controls the browser, runs TART and then closes it. I can't do it from the main browser window because TART keeps opening and closing tabs. I tried opening a new window to check about:config from there but the browser immediately closes/crashes with a message at the console "FATAL ERROR: Non-local network connections are disabled and a connection attempt to snippets.mozilla.com ...".

So I couldn't really check it.

Did you try running talos locally with your patch and observing the screen, confirming that it works as you expect it to?
(In reply to Avi Halachmi (:avih) from comment #31)
> > Can you check what browser.newtabpage.* preferences are? In particular
> > browser.newtabpage.directory.source
> > 
> > And if that has a localhost file, what if you try loading that page in the
> > browser?
> 
> Really hard to check.

I managed it anyway (quickly before the test starts by pasting this directly: about:config?filter=/browser.newtabpage.directory.source/ ).

browser.newtabpage.directory.source = http://localhost:15707/directoryLinks.json

And that at that URL is a json file which looks like the one at your patch.

If I open a new tab before the TART starts, then I see 1 white tile and below written "shutdown script" (without the quotes), and 5 empty dashed frames (there are about 5 seconds after the browser opens and before the tests start - so I used that idle to open few new tabs - all look the same).
For convenience, that's the command line to run talos with TART only (5 times in a row):

> talos -e path/to/firefox.exe --develop --tppagecycles=5 --tpcycles=1 -a tart
I wonder if it's because the remote file is cached locally, so when I ran it, it happened to have it cached... I'll take a look in the morning.
You need to log in before you can comment on or make changes to this bug.