Closed Bug 719455 Opened 9 years ago Closed 8 years ago

TestPilot in mozilla-central is 1.1.2; should be 1.2 (Critical)

Categories

(Mozilla Labs Graveyard :: Test Pilot, defect)

Product:
Mozilla Labs Graveyard
Component:
Test Pilot
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: glind, Assigned: glind)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 file, 4 obsolete files)

removed old css, xhr.js
188.88 KB, patch
dao
: review+
Details | Diff | Splinter Review 
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.
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.)
Duplicate of this bug: 711042
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?
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]
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.
So this leak affects 13% of our aurora/beta audience?  I'd call that a MemShrink:P1.
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 :-)
Especially since it's not uncommon for someone to install Aurora or Beta for a quick performance / memory usage test.
Summary: TP 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)
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! :)
(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.
(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.
Assignee: glind → gavin.sharp
Status: NEW → ASSIGNED
Not sure why you assigned this to me... Did you mean to ask me for review?
Assignee: gavin.sharp → nobody
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.
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)
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.)
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?
Gavin, please review the attached diff for inclusion.  ETA for this?
> Attachment #608489 [details] [diff] - Flags: review-

Did you mean to set r? rather than r-  ?
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.
Attachment #608489 - Flags: review? → review?(gavin.sharp)
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 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?
Attachment #608489 - Flags: review?(gavin.sharp) → review-
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.
Since this seems to be back on the radar, I will review this by Monday, and resolve changes.
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?
How about |diff -U8 -R path1 path2|?  You may have to futz further with those params, but that's the general idea...
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`
Attachment #608489 - Attachment is obsolete: true
Attachment #635128 - Flags: review?
(I am on vaca this week, but will be around to answer the review questions on this.)
Attachment #635128 - Flags: review? → review?(gavin.sharp)
Gavin, if there is a more appropriate reviewer, let me know :)  Thanks for the patience!
Attachment #635128 - Flags: review?(gavin.sharp) → review?(dao)
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)
Attachment #635128 - Flags: review?(dao) → review-
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:
Gavin, noted and corrected. 

Dao, I don't really know how to turn your comment (30) into code, or exactly what to touch there.
(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 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?
Attachment #655140 - Flags: review?(dao) → review-
Blocks: 797211
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.
> 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!
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.
Revised to remove non-existing fonts;  removes xhr.js
Attachment #635128 - Attachment is obsolete: true
Attachment #655140 - Attachment is obsolete: true
Attachment #655140 - Flags: review?(gavin.sharp)
Attachment #667695 - Flags: review?(dao)
Attachment #667695 - Flags: review?(dao) → review+
Thanks for getting to this so quickly Dão!  Do I need to take next steps here?
Marking this checkin-needed is the way to get your reviewed patches committed, when you don't have level 3 commit access.
Keywords: checkin-needed
Assignee: nobody → glind
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5136ce893ba9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Thanks to everyone for getting this resolved!  In the future, things will land much more quickly.
Product: Mozilla Labs → Mozilla Labs Graveyard
You need to log in before you can comment on or make changes to this bug.