Last Comment Bug 719455 - TestPilot in mozilla-central is 1.1.2; should be 1.2 (Critical)
: TestPilot in mozilla-central is 1.1.2; should be 1.2 (Critical)
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Mozilla Labs Graveyard
Classification: Graveyard
Component: Test Pilot (show other bugs)
: unspecified
: x86 All
: -- major (vote)
: ---
Assigned To: Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
:
Mentors:
: 711042 (view as bug list)
Depends on:
Blocks: 797211
  Show dependency treegraph
 
Reported: 2012-01-19 08:49 PST by Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
Modified: 2016-05-10 13:13 PDT (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release. (6.24 KB, patch)
2012-03-22 14:43 PDT, Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
no flags Details | Diff | Review
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release. (74.40 KB, patch)
2012-03-22 14:56 PDT, Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
gavin.sharp: review-
Details | Diff | Review
Revised patch. Style sheets, debug, fonts etc. (97.60 KB, patch)
2012-06-20 16:51 PDT, Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
dao+bmo: review-
Details | Diff | Review
revised for bottom-border-colors (189.42 KB, patch)
2012-08-24 13:54 PDT, Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
dao+bmo: review-
Details | Diff | Review
removed old css, xhr.js (188.88 KB, patch)
2012-10-03 15:21 PDT, Gregg Lind (User Advocacy - Heartbeat - Test Pilot)
dao+bmo: review+
Details | Diff | Review

Description Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-01-19 08:49:15 PST
mozilla-central/browser/app/profile/extensions/testpilot@labs.mozilla.com  should have 1.2, but has 1.1.2.  


Why this is a problem:

1)  There are deployed studies that depend on 1.2 specific features.

Redress Plan:

1)  Check that all patches that were in Gavin's lap actually got landed.  If not, why not?
2)  Find a list of relevent patch bugs
3)  Determine a minimal patch-set that solves the problem.  
4)  Land this ASAP in affected channels.
Comment 1 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-01-30 11:37:09 PST
1.  downloaded:

    https://addons.mozilla.org/firefox/downloads/file/133851/test_pilot-1.2-fx-mac.xpi

2.  from the test pilot source from http://hg.mozilla.org/labs/testpilot/, 
    ``f642cac3dd28`` seems to be right.

3.  (Still working out the proper diffset.)
Comment 2 Justin Lebar (not reading bugmail) 2012-02-19 16:18:30 PST
*** Bug 711042 has been marked as a duplicate of this bug. ***
Comment 3 Justin Lebar (not reading bugmail) 2012-02-19 16:19:53 PST
In bug 711042, we found that older versions of TestPilot leak memory.

Jinghua says there are 3M people on TP 1.2.  How many people are running an older version?
Comment 4 Jinghua zhang 2012-02-21 17:34:04 PST
we now have 3.2M people TP 1.2, plus about 160K people on TP 1.1.2, 140K people on TP 1.0.3, and 200K on other TP versions.
Comment 5 Justin Lebar (not reading bugmail) 2012-02-22 02:41:58 PST
So this leak affects 13% of our aurora/beta audience?  I'd call that a MemShrink:P1.
Comment 6 Mark Banner (:standard8) 2012-02-22 03:47:20 PST
Note: Firefox only distributes (not enforces) 1.1.2 to aurora/beta, within a day or so, depending on the settings the Add-on Manager should be updating people to 1.2 the version on AMO.

So for those that are letting the add-on manager do its job, this should be a short-lived issue. It still should be fixed, of course :-)
Comment 7 Justin Lebar (not reading bugmail) 2012-02-22 04:01:27 PST
Especially since it's not uncommon for someone to install Aurora or Beta for a quick performance / memory usage test.
Comment 8 Nicholas Nethercote [:njn] 2012-03-13 17:35:04 PDT
What's the status of this?  It's unclear to me.

(In reply to Mark Banner (:standard8) from comment #6)
> Note: Firefox only distributes (not enforces) 1.1.2 to aurora/beta, within a
> day or so, depending on the settings the Add-on Manager should be updating
> people to 1.2 the version on AMO.

What settings are involved?  In other words, which users are likely to be stuck on 1.1.2?  Is it only people who have changed an unusual option, e.g. one buried in about:config?  Or is it something that's more common?


(In reply to Jinghua zhang from comment #4)
> we now have 3.2M people TP 1.2, plus about 160K people on TP 1.1.2, 140K
> people on TP 1.0.3, and 200K on other TP versions.

Those numbers are now 3+ weeks old.  Jinghua, could you post new numbers?  I'd really like to close this bug! :)
Comment 9 Dave Townsend [:mossop] 2012-03-13 18:29:10 PDT
(In reply to Nicholas Nethercote [:njn] from comment #8)
> What's the status of this?  It's unclear to me.

The status is that we're waiting on the Test Pilot team to provide patches for us to approve and get landed in m-c.

> (In reply to Mark Banner (:standard8) from comment #6)
> > Note: Firefox only distributes (not enforces) 1.1.2 to aurora/beta, within a
> > day or so, depending on the settings the Add-on Manager should be updating
> > people to 1.2 the version on AMO.
> 
> What settings are involved?  In other words, which users are likely to be
> stuck on 1.1.2?  Is it only people who have changed an unusual option, e.g.
> one buried in about:config?  Or is it something that's more common?

The vast majority of users have automatic updates enabled, it can be disabled through Firefox's options or in the add-ons manager UI.
Comment 10 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-03-22 14:43:11 PDT
Created attachment 608483 [details] [diff] [review]
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release.
Comment 11 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-03-22 14:43:49 PDT
(In reply to Gregg Lind (User Research - Test Pilot) from comment #10)
> Created attachment 608483 [details] [diff] [review]
> mondo patch of trunk (1.1ish) vs 1.2.1 AMO release.

This should get reviewed by Jono before going to the wolves.
Comment 12 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-03-22 14:56:07 PDT
Created attachment 608489 [details] [diff] [review]
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-22 15:10:54 PDT
Not sure why you assigned this to me... Did you mean to ask me for review?
Comment 14 Jono Xia 2012-03-22 16:32:00 PDT
Looks like some stuff is in that patch that doesn't need to be -- lib/xhr.js and lib/file.js aren't used anywhere, so we should remove them from TP 1.2 rather than trying to add them to mozilla-central.
Comment 15 Jono Xia 2012-03-22 16:41:07 PDT
Looking at the patch some more, I'm not sure that the chrome.manifest changes should be included? I think the chrome.manifest that was in mozilla-central was working fine, so if those two lines that are there in the labs hg repo are missing in mozilla-central, it's probably for a good reason. We might have to tolerate some small differences between the repos in chrome.manifest (and install.rdf / install.rdf.in)
Comment 16 Jono Xia 2012-03-22 16:46:09 PDT
Actually, it looks like one of the two lines changed in chrome.manifest (the one that applies a style to the customize panel) is needed, the other (locale) isn't (because the localization files are in a different place in labs hg vs. mozilla-central.)
Comment 17 Justin Lebar (not reading bugmail) 2012-04-03 16:52:53 PDT
Speaking for MemShrink, we would really like this to be fixed.  We're leaving a bad first impression by shipping a leaky add-on with our pre-release builds, even if the leak gets fixed via the add-on update mechanism.

The 1.2 code is already shipping to users via AMO, so I presume these reviews for landing in m-c are perfunctory?

Gregg, is this still on your radar?
Comment 18 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-04-04 09:24:42 PDT
Gavin, please review the attached diff for inclusion.  ETA for this?
Comment 19 Justin Lebar (not reading bugmail) 2012-04-04 09:32:13 PDT
> Attachment #608489 [details] [diff] - Flags: review-

Did you mean to set r? rather than r-  ?
Comment 20 Andrew McCreight [:mccr8] 2012-04-04 09:35:16 PDT
Comment on attachment 608489 [details] [diff] [review]
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release.

You should set the review field when you ask for a review.
Comment 21 Dão Gottwald [:dao] 2012-05-01 16:29:29 PDT
Comment on attachment 608489 [details] [diff] [review]
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release.

> .button {
> 	font-family: 'DroidSans';
> 	font-size: 16px;
> 	padding: 8px 12px;
> 	color: rgba(0, 0, 0, 0.8);
>-	border-radius: 0.5em;
>-	box-shadow: 
>+	-moz-border-radius: 0.5em;
>+	-webkit-border-radius: 0.5em;
>+	-moz-box-shadow: 

This change looks completely wrong...
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-14 19:25:06 PDT
Comment on attachment 608489 [details] [diff] [review]
mondo patch of trunk (1.1ish) vs 1.2.1 AMO release.

I'm sorry I haven't gotten to this review request in a timely fashion. There are a bunch of things in this patch that look wrong to me or that I don't understand.

As dao points out, the changes to screen.css look wrong. Was this some problem with how the patch was produced, or does it point to an existing problem in the add-on version?

The files in modules/lib/ and notifications.js don't seem to be used at all.

The screen-standalone.css changes add webkit-only styles - I don't understand why we'd want any of those.

Gregg, could you shed some light on this?
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-06-03 21:34:41 PDT
Gregg, can you answer Gavin's questions in comment 22?

On another note, it's very disappointing that we're into our fifth month of trying to move some code from one repository to another.  If we're not going to keep the copy of Test Pilot in mozilla-central up to date I start to wonder why Test Pilot is in mozilla-central at all.
Comment 24 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-06-13 13:45:16 PDT
Since this seems to be back on the radar, I will review this by Monday, and resolve changes.
Comment 25 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-06-19 15:22:55 PDT
I am stalled here, because I don't really remember how I mad the diff in the first place! 

I know there is some elegant way of doing the diff, but it's failing me.  

  path1:  ~/hgs/mozilla-central/browser/app/profile/extensions/testpilot@labs.mozilla.com
  path2:  ~/hgs/testpilot/extension   # the labs repo

Is there some combo of `hg pull -f` or `hg diff` that does this diff right?
Comment 26 Justin Lebar (not reading bugmail) 2012-06-19 16:50:05 PDT
How about |diff -U8 -R path1 path2|?  You may have to futz further with those params, but that's the general idea...
Comment 27 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-06-20 16:51:30 PDT
Created attachment 635128 [details] [diff] [review]
Revised patch.  Style sheets, debug, fonts etc.

Specific notes:

1.  Locale stuff might be wrong. (feedback vs. testpilot)  
2.  I think I got all '-moz' and 'webkit-' instances that can be resolved.
3.  Holler if there a built version I can try out with debug settings on.
4.  The `debug.html` page has a link to `jquery` now, which is a bit gross.  Make it more version agnostic if you like, or kick it back.
5.  Sorry there aren't automated tests :(.  TP2 will have some.
6.  This version will be 1.2.2 on AMO when it is approved.
7.  check that all files have right licenses.



----
How patch was built (for future laborers):

    # before beginning, get D1 and D2 to the rev you want.

    D1=~/hgs/mozilla-central/browser/app/profile/extensions/testpilot@labs.mozilla.com
    D2=~/hgs/testpilot/extension
    DG=~/gits/tpmerge
    ## git init...
    rm -rf "$DG"
    git init "$DG"
    
    # rsync the first dir
    cd "$D1" && (hg status -c -n . | xargs -I{} rsync -avR "{}" "$DG")
    
    # checkin
    cd "$DG" && git add * && git commit -m "from D1"
    
    # D2!
    cd "$D2" && (hg status -c -n . | xargs -I{} rsync -avR --del "{}" "$DG")
    
    cd "$DG"  
    echo "git status;  git add -p;  git diff;  etc!"

    # then used `git-add -p` over several iterations, commited
    # then `git diff -pU8 HEAD~1 HEAD > bug.patch`
Comment 28 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-07-02 08:52:49 PDT
(I am on vaca this week, but will be around to answer the review questions on this.)
Comment 29 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-07-09 14:08:07 PDT
Gavin, if there is a more appropriate reviewer, let me know :)  Thanks for the patience!
Comment 30 Dão Gottwald [:dao] 2012-07-13 07:24:52 PDT
Comment on attachment 635128 [details] [diff] [review]
Revised patch.  Style sheets, debug, fonts etc.

> /* Popup Bounding Box */
> #pilot-notification-popup {
>   -moz-appearance: none;
>   -moz-window-shadow: none;
>   background-color: transparent;
>   margin-top: -6px;
>   margin-right: -3px;
>   width: 480px;
>+  /* FIXES: #725850  based on #717262
>+  /* Needed whilst we support Gecko < 13 */
>+  border-image: url(chrome://testpilot-os/skin/notification-tail-up.png) 26 56 22 18 / 26px 56px 22px 18px round stretch;
>+  /* Supported in Gecko >= 13 */
>+  border-image: url(chrome://testpilot-os/skin/notification-tail-up.png) 26 50 22 18 fill stretch;
>+  border-width: 26px 56px 22px 18px;
>+  border-style: solid;
> }

It looks like you just want <panel type="arrow"> here. Please file a bug on using that in case you don't want to do it in this bug.

> #menu {
> 	margin: 20px auto;
> 	max-width: 800px;
> 	padding: 4px 40px;
> 	width: 800px;
> 	text-align: left;
> 	border-radius: 0.25em;
> 	border-top: 1px solid #adb6ba;
> 	border-left: 1px solid #adb6ba;
> 	border-right: 1px solid #adb6ba;
> 	border-bottom: 3px solid #adb6ba;
>-	-moz-border-bottom-colors:#adb6ba #e7eaec #e7eaec;
>+	border-bottom-colors:#adb6ba #e7eaec #e7eaec;

border-bottom-colors doesn't exist (here and elsewhere in this patch)
Comment 31 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-06 15:13:18 PDT
Comment on attachment 635128 [details] [diff] [review]
Revised patch.  Style sheets, debug, fonts etc.

One other issue I noticed:
- I know debug.html is just for debugging and isn't used by normal users, but it really shouldn't load jQuery from a remote host and run that in a chrome context:
Comment 32 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-08-20 08:39:06 PDT
Gavin, noted and corrected. 

Dao, I don't really know how to turn your comment (30) into code, or exactly what to touch there.
Comment 33 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-24 12:39:12 PDT
(In reply to Gregg Lind (User Research - Test Pilot) from comment #32)
> Dao, I don't really know how to turn your comment (30) into code, or exactly
> what to touch there.

You just need to remove the invalid CSS he was referencing. The followup bug can be filed separately.
Comment 34 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-08-24 13:54:56 PDT
Created attachment 655140 [details] [diff] [review]
revised for bottom-border-colors
Comment 35 Dão Gottwald [:dao] 2012-09-01 05:13:49 PDT
Comment on attachment 655140 [details] [diff] [review]
revised for bottom-border-colors

>-	
>-	.bold {font-family: 'DroidSans-Bold'; color:#3a3a3a;}
>+
>+	.bold {font-family: 'sans-serif-Bold'; color:#3a3a3a;}

>-.downloadH1 {font-family: 'DroidSans-bold'; color: #3a3a3a; font-size: 24px; font-weight: normal; letter-spacing: -1px; margin-right: 8px;}
>+.downloadH1 {font-family: 'sans-serif-bold'; color: #3a3a3a; font-size: 24px; font-weight: normal; letter-spacing: -1px; margin-right: 8px;}
> 
>-.downloadH2 {font-family: 'DroidSans'; color: #3a3a3a; font-size: 22px; font-weight: normal; letter-spacing: -1px; line-height: 28px; margin-left: 46px;}
>+.downloadH2 {font-family: 'sans-serif'; color: #3a3a3a; font-size: 22px; font-weight: normal; letter-spacing: -1px; line-height: 28px; margin-left: 46px;}

sans-serif-bold is not a valid font family. It looks like you should just adjust font-size and font-weight as needed and leave font-family alone (i.e. drop it from the code quoted above).

Is TestPilot supposed to work in builds localized in right-to-left languages?
Comment 36 Jorge Villalobos [:jorgev] 2012-10-02 17:14:24 PDT
Test Pilot version 1.1.2 has 256K ADU and rising, and it is known to cause memory leaks because it's using an old version of the SDK. We have known this for almost a year now.

Can we please move this along? We can file separate bugs with RTL and CSS issues to follow up in future versions. I think they are minor problems considering the reasons this is urgent in the first place.
Comment 37 Nicholas Nethercote [:njn] 2012-10-02 17:46:54 PDT
> Can we please move this along?

Indeed.  We just discussed this very bug an hour ago in our MemShrink meeting and I came here to say exactly the same thing.  It's disgraceful that we're shipping this to some of our most important users (aurora and beta users).

Come on, let's get this done, please!
Comment 38 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-10-03 10:27:55 PDT
What would help me in getting this done, is identifying *any and all outstanding css problems*, where 

1.  I am the weakest
2.  It's the hardest to test.

Getting them back one at a time means weeks between updates.

We are on 1.2.3 on AMO, and I am trying to diff to that.
Comment 39 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-10-03 15:21:57 PDT
Created attachment 667695 [details] [diff] [review]
removed old css, xhr.js

Revised to remove non-existing fonts;  removes xhr.js
Comment 40 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-10-04 09:06:45 PDT
Thanks for getting to this so quickly Dão!  Do I need to take next steps here?
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-07 08:25:26 PDT
Marking this checkin-needed is the way to get your reviewed patches committed, when you don't have level 3 commit access.
Comment 42 Justin Lebar (not reading bugmail) 2012-10-07 09:13:46 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #41)
> Marking this checkin-needed is the way to get your reviewed patches
> committed, when you don't have level 3 commit access.

Except this isn't a patch to m-c -- it's a patch to browser/app/profile/extensions/testpilot@labs.mozilla.com -- and it's also missing the appropriate hg headers.

To apply the patch, I did

$ cd browser/app/profile/extensions/testpilot@labs.mozilla.com
$ curl -L https://bugzilla.mozilla.org/attachment.cgi?id=667695 | patch --strip 1
Hunk #1 FAILED at 156.
1 out of 1 hunk FAILED -- saving rejects to file content/experiment-page.js.rej

This reject comes from bug 794602.  I resolved the reject, but I hope the TP master repository has been updated to reflect the change from bug 794602.

Gregg, please see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed in the future.

Anywho, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5136ce893ba9.
Comment 43 Phil Ringnalda (:philor) 2012-10-07 18:37:36 PDT
https://hg.mozilla.org/mozilla-central/rev/5136ce893ba9
Comment 44 Honza Bambas (:mayhemer) 2012-10-09 10:59:10 PDT
You've by accident landed a .rej file with your patch, please remove it:

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/testpilot@labs.mozilla.com/content/experiment-page.js.rej
Comment 45 Justin Lebar (not reading bugmail) 2012-10-09 11:40:35 PDT
Fixed, and thanks.  https://hg.mozilla.org/integration/mozilla-inbound/rev/ea63926c22cb
Comment 46 Ryan VanderMeulen [:RyanVM] 2012-10-09 18:32:12 PDT
https://hg.mozilla.org/mozilla-central/rev/ea63926c22cb
Comment 47 Gregg Lind (User Advocacy - Heartbeat - Test Pilot) 2012-10-11 10:36:18 PDT
Thanks to everyone for getting this resolved!  In the future, things will land much more quickly.
Comment 48 Justin Lebar (not reading bugmail) 2012-10-13 07:22:00 PDT
And removed the .rej/.orig file from Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/2c579668df52

Note You need to log in before you can comment on or make changes to this bug.