855371 - @MOZ_UPDATE_CHANNEL@ in health report prefs

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

I opened about:config in today's Nightly and noticed that @MOZ_UPDATE_CHANNEL@ is in a healthreport pref:

datareporting.healthreport.service.providerCategories;healthreport-js-provider-default,healthreport-js-provider-@MOZ_UPDATE_CHANNEL@

That file (getting merged into greprefs.js) is supposed to be preprocessed. And, I thought this was verified to work in bug 853138. Not sure what's going on here. Gah.

Comment 1

[Richard Newman [:rnewman]]

Yeah, I verified this. humph.

Comment 2

[Gregory Szorc [:gps]]

The only thing I can think of is that if you switch a file from copy to preprocess, that effectively changes a symlink to a new file. When we write a file on top of an existing symlink, bad things can happen. Clobber fixes that. However, Nightly builds should be clobber, so...

Comment 3

[Richard Newman [:rnewman]]

So I'm pretty sure I've done some clobber builds in the past week, and yet greprefs.js:

pref("datareporting.healthreport.service.providerCategories",
//@line 28 "/Users/rnewman/moz/hg/services-central/services/healthreport/healthreport-prefs.js"
    "healthreport-js-provider-default"
//@line 32 "/Users/rnewman/moz/hg/services-central/services/healthreport/healthreport-prefs.js"
    );

but in my Nightly:

pref("datareporting.healthreport.service.providerCategories",
//@line 30 "/builds/slave/m-cen-osx64-ntly-0000000000000/build/services/healthreport/healthreport-prefs.js"
    "healthreport-js-provider-default,healthreport-js-provider-@MOZ_UPDATE_CHANNEL@"
//@line 32 "/builds/slave/m-cen-osx64-ntly-0000000000000/build/services/healthreport/healthreport-prefs.js"
    );


That means that the file is being preprocessed, but it's not being substituted correctly.

Comment 4

[Richard Newman [:rnewman]]

Just did a clobber build and package. greprefs in my omni.ja is preprocessed correctly.

Comment 5

[Richard Newman [:rnewman]]

Though I should note that I'm using update-channel=release, so I'm not actually testing the substitution.

Comment 6

[Richard Newman [:rnewman]]

Guessed it might be a missing #filter substitution, and gavin confirmed.

The fun now is that custom builds look like this:

//@line 27 "/Users/rnewman/moz/hg/services-central/services/healthreport/healthreport-prefs.js"
pref("datareporting.healthreport.service.providerCategories",
//@line 31 "/Users/rnewman/moz/hg/services-central/services/healthreport/healthreport-prefs.js"
    "healthreport-js-provider-default,healthreport-js-provider-default"
//@line 33 "/Users/rnewman/moz/hg/services-central/services/healthreport/healthreport-prefs.js"
    );

so we might need to hack up that conditional some more.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Keep it simple.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 734071 [details] [diff] [review]
Proposed patch. v1

I'd put the #filter line at the top of the file, since that's where people usually put it (and it applies to the whole file).

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 734071 [details] [diff] [review]
Proposed patch. v1

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

::: services/healthreport/healthreport-prefs.js
@@ +22,5 @@
>  pref("datareporting.healthreport.service.enabled", true);
>  pref("datareporting.healthreport.service.loadDelayMsec", 10000);
>  pref("datareporting.healthreport.service.loadDelayFirstRunMsec", 60000);
>  
> +#filter substitution

Put this at the top of the file since it applies to the entire file.

Comment 10

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/225be6d5e41b

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/225be6d5e41b

Comment 12

[Richard Newman [:rnewman]]

Attachment: Patch for uplift.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  853138

User impact if declined: 
  None visible, but prefs will contain a weird value.

Testing completed (on m-c, etc.): 
  Landed on m-c. No breakage on s-c, where it's been for a while.

Risk to taking this patch (and alternatives if risky): 
  Very low.
 
String or IDL/UUID changes made by this patch:
  None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdec0ba6a574

Comment 14

[u279076]

(In reply to Gregory Szorc [:gps] from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/225be6d5e41b

Verified fixed in Firefox Nightly 23.0a1 2013-04-17.

Comment 15

[u279076]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/cdec0ba6a574

Verified fixed in Firefox Aurora 22.0a2 2013-04-18.