720997 - add sync prefs prefix to about:support whitelist

Status: NEW

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

useful for debugging, we may want to be slightly more selective, but not sure.

Comment 1

[Allison Naaktgeboren :ally]

connor et al, thoughts on how selective you'd like to be?

Comment 2

[Gregory Szorc [:gps]]

account, username, *URL, engine enabled flags, *lastSync, *lastSyncLocal are a good start.

Comment 3

[Mike Connor [:mconnor]]

do we really want lastSync*?  I'm not sure at all.

Comment 4

[Richard Newman [:rnewman]]

Sync timestamps sound pretty handy for debugging imo.

Comment 5

[Allison Naaktgeboren :ally]

any others?

Comment 6

[Allison Naaktgeboren :ally]

Attachment: wip/proof of concept part 1/1, v0

- There are two outstanding questions (see comments). One deals with branding in the localization & the other asks if there is a more elegant way to iterate through the sync prefs.

- the output is currently missing headers. Unclear if we want them. Im inclined to want them for consistency with all the other tables.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 659040 [details] [diff] [review]
wip/proof of concept part 1/1, v0

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

The bug originally describes adding sync prefs to the whitelist, but this adds an entirely new section. I'd rather have it just added it to the existing whitelist, and keep all the prefs grouped together (other features haven't had their prefs been split off to their own sections) - but mconnor should weigh in there.

::: toolkit/content/aboutSupport.js
@@ +434,5 @@
> +      createElement("td", getPrefValue(prefName).value),
> +      ]);
> +    table.push(tr);
> +  }
> + // anaaktge :Reviewer, pretty please tell me theres a better way!

Hmm, yea, should do this in a way such that any new engine automatically has its prefs listed in about:support - because it will be forgotten (also good to handle 3rd party engines, IMO). Can handle that via a couple of regular expressions, something like:
  /^services\.sync\.engine\.[^.]+$/
  /^services\.sync\.[^.]+\.(lastSync|lastSyncLocal)$/

Can detect a regexp via |instanceof RegExp|, and passing an empty string to .getChildList() will return all prefs.

@@ +476,5 @@
> +    "services.sync.tabs.lastSyncLocal",
> +  ];
> +
> +  let trEntries = [];
> +  for(var i=0; i < syncWhiteList.length; ++i) {

for..of loops are the way of teh future!

  for (let item of syncWhiteList)
    ...

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +60,4 @@
>  <!ENTITY aboutSupport.installationHistoryTitle "Installation History">
>  <!ENTITY aboutSupport.updateHistoryTitle "Update History">
>  
> +<!-- anaaktge :Reviewer, does this need the Fxsync localized branding pulled in? -->

If its to be in its own section, then yes.

Comment 8

[Allison Naaktgeboren :ally]

Attachment: v1, an updated diff (regex, waiting on mconnor i/o)

Blair, is this more of what you had in mind for the regex?

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 660917 [details] [diff] [review]
v1, an updated diff (regex, waiting on mconnor i/o)

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

Yep :)

::: toolkit/content/aboutSupport.js
@@ +451,5 @@
> +    "services.sync.username",
> +    "services.sync.clusterURL",
> +    "services.sync.jpake.serverURL",
> +    new RegExp("^services\.sync\.engine\.[^.]+$"),
> +    new RegExp("^services\.sync\.[^.]+\.(lastSync|lastSyncLocal)$"),

You don't need to explicitly use the RegExp constructor here, JS supports constructing regular expressions as literals. ie: 

  "services.sync.jpake.serverURL",
  /^services\.sync\.engine\.[^.]+$/,
  /^services\.sync\.[^.]+\.(lastSync|lastSyncLocal)$/


(Everything else works exactly the same - this just makes it less verbose/easier to read.)

Comment 10

[Allison Naaktgeboren :ally]

(In reply to Blair McBride (:Unfocused) from comment #9)
> Comment on attachment 660917 [details] [diff] [review]
> 
> (Everything else works exactly the same - this just makes it less
> verbose/easier to read.)

Sweet! Maybe I should send things to you for review more often... I learn all sorts of nifty js things. :D

Comment 11

[Mike Connor [:mconnor]]

Third attempt at writing this comment:

I think we should have a Sync section, for two reasons:

* We'll overwhelm the "important prefs" section for a lot of users if we don't split it out.  For me it'd be about 150% increase.
* I think we should have a followup to add a button linking to "Sync Error Logs" (about:sync-log) since that's the next logical step in troubleshooting.

Comment 12

[Allison Naaktgeboren :ally]

Attachment: v2 part 1/1

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 662322 [details] [diff] [review]
v2 part 1/1

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

Congrats, you've been horribly bitrotten by bug 554174 and bug 789674! All the stuff needs to go in toolkit/content/Troubleshoot.jsm now.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #13)
> All the stuff needs to go in toolkit/content/Troubleshoot.jsm now.

Er, all the data collection stuff, that is. Still need a data formatter in aboutSupport.js, of course.

Comment 15

[Allison Naaktgeboren :ally]

Attachment: v3 part 1/1 - rewritten

note for the dtd file: hg's conflict resolution there is kind of silly.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 664605 [details] [diff] [review]
v3 part 1/1 - rewritten

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

::: toolkit/content/Troubleshoot.jsm
@@ +356,5 @@
>      }
>      done(data);
>    },
> +
> +  syncDiagnostics: function syncDiagnostics(done) {

All of Troubleshoot.jsm is about diagnostics, so I'd name this simply "sync" or "syncService" or something.

@@ +371,5 @@
> +        /^services\.sync\.[^.]+\.(lastSync|lastSyncLocal)$/,
> +      ];
> +
> +      for (let entry of syncWhiteList) {
> +        if(entry instanceof RegExp) {

Nit: space between "if" and the opening parenthesis.

@@ +372,5 @@
> +      ];
> +
> +      for (let entry of syncWhiteList) {
> +        if(entry instanceof RegExp) {
> +          for (let pref of allprefs) {

It doesn't look like allprefs is defined anywhere, and looking at all preferences, if that's what you really mean, to match a handful of preferences whose branches you know beforehand would be really inefficient.

@@ +373,5 @@
> +
> +      for (let entry of syncWhiteList) {
> +        if(entry instanceof RegExp) {
> +          for (let pref of allprefs) {
> +            if(entry.test(pref)) {

Nit: space here too.  Also, this file doesn't use braces where they aren't strictly necessary.

::: toolkit/content/tests/browser/browser_Troubleshoot.js
@@ +132,5 @@
> +    syncDiagnostics: {
> +      required: true,
> +      type: "object",
> +      properties: {
> +        account: {

It looks like the properties that end up in the syncDiagnostics object are named "services.sync.account", etc., not simply "account".  If it's the case that none of these properties are required, then to make sure syncDiagnostics really looks like you expect it to look, you could add a new test function to the tests array that adds various services.sync.* prefs, gets a snapshot, and then checks the snapshot by hand.  (You could make a mini schema and use validateObject to check only the syncDiagnostics object.)

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +61,5 @@
>  <!ENTITY aboutSupport.updateHistoryTitle "Update History">
>  
>  <!ENTITY aboutSupport.copyTextToClipboard.label "Copy text to clipboard">
>  <!ENTITY aboutSupport.copyRawDataToClipboard.label "Copy raw data to clipboard">
> +<!ENTITY aboutSupport.copyToClipboard.label "Copy all to clipboard">

Did you mean to add this back?  (Or is it what you meant by hg's conflict resolution?)

Comment 17

[Allison Naaktgeboren :ally]

(In reply to Drew Willcoxon :adw from comment #16)
> Comment on attachment 664605 [details] [diff] [review]
> v3 part 1/1 - rewritten
> 
> Review of attachment 664605 [details] [diff] [review]:
> -----------------------------------------------------------------

> It doesn't look like allprefs is defined anywhere, and looking at all
> preferences, if that's what you really mean, to match a handful of
> preferences whose branches you know beforehand would be really inefficient.

It is the procedure :Unfocused asked for in comment 7.

"let allprefs = Services.prefs.getChildList("");" ln373 ? I see it in my branch. However, I don't see it in the submitted diff. I must have fatfingered it.

> > +<!ENTITY aboutSupport.copyToClipboard.label "Copy all to clipboard">
> 
> Did you mean to add this back?  (Or is it what you meant by hg's conflict
> resolution?)

Yup. I neither added or removed that line, but I did append lines. That's how it resolved the aboutSupport.syncPrefsName & aboutSupport.copyToClipboard conflict.

Comment 18

[Drew Willcoxon :adw]

He doesn't suggest enumerating through all preferences in the app, which is what this does:

> "let allprefs = Services.prefs.getChildList("");"

You want to either start at the "services.sync." branch and then call getChildList, or pass "services.sync." to getChildList.  Either way, you want to enumerate over only the services.sync.* prefs.

> Yup. I neither added or removed that line, but I did append lines. That's
> how it resolved the aboutSupport.syncPrefsName &
> aboutSupport.copyToClipboard conflict.

OK, but it shouldn't be in the file, so you should remove it.

Comment 19

[Allison Naaktgeboren :ally]

(In reply to Drew Willcoxon :adw from comment #18)
> He doesn't suggest enumerating through all preferences in the app, which is
> what this does:

"Can detect a regexp via |instanceof RegExp|, and passing an empty string to .getChildList() will return all prefs." - comment 7. 

I am confused then; please help me understand how this is *not* suggesting I enumerate through all the prefs? 

I have no strong preference either way. :Unfocused, what would you like?

Comment 20

[Allison Naaktgeboren :ally]

Attachment: v4 part 1/1

- feedback addressed
- has the restricted allprefs

:Unfocused, is there a reason not to do the restricted child search as :adw suggests?

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 664704 [details] [diff] [review]
v4 part 1/1

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

(In reply to :Ally Naaktgeboren from comment #20)
> :Unfocused, is there a reason not to do the restricted child search as :adw
> suggests?

Nope. I would have been more appropriate if we added the Sync prefs to the main preferences section, but since it has its own section now...

::: toolkit/content/Troubleshoot.jsm
@@ +386,5 @@
> +
> +    done(data);
> +
> +    function makeRow(name) {
> +      data[name] = getPref(name);

This doesn't really need to be split into its own function.

::: toolkit/content/aboutSupport.js
@@ +180,5 @@
>    },
> +
> +  syncService: function syncService(data) {
> +    let trs = [];
> +    sortedArrayFromObject(data).forEach(

Since you're now making it so data only contains anything if Sync is configured, if it's *not* configured this currently results an empty table. In that case, it should state that it's not configured.

Can either handle that here, or make the data provider return something like:
  {
    status: whatever,
    prefs: {
      whatever: whatever,
      ...
    }
  }


I think including the status in the data provider would be useful anyway.

@@ +181,5 @@
> +
> +  syncService: function syncService(data) {
> +    let trs = [];
> +    sortedArrayFromObject(data).forEach(
> +      function ([name, val]) {

Nit: This file doesn't seem to use them, but for-of loops are still the new hotness :)

  for (let [name, val] of sortedArrayFromObject(data)) {
    ...
  }

::: toolkit/content/tests/browser/browser_Troubleshoot.js
@@ +130,5 @@
>        type: "object",
>      },
> +    syncService: {
> +      required: true,
> +      type: "object",

Is there any way to test this more fully, as suggested in comment 16, without messing with Sync too much?

Comment 22

[Allison Naaktgeboren :ally]

(In reply to Blair McBride (:Unfocused) from comment #21)
> Comment on attachment 664704 [details] [diff] [review]
> v4 part 1/1
> 
> Review of attachment 664704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/tests/browser/browser_Troubleshoot.js
> @@ +130,5 @@
> >        type: "object",
> >      },
> > +    syncService: {
> > +      required: true,
> > +      type: "object",
> 
> Is there any way to test this more fully, as suggested in comment 16,
> without messing with Sync too much?

:adw & I had talked earlier today and agreed that there currently isn't.

Comment 23

[Drew Willcoxon :adw]

Well, to be clear it's not that I think it couldn't be done, it's that I didn't think it's worth testing.  But I gave it a second thought after Ally and I talked, and I think it is worth testing because it could be the case that the data provider is actually not capturing services.sync.* prefs at all, and if there's no test we'd never know.

So what I'd do, for whatever it's worth, is have status and preferences properties like Blair suggests, and have the data provider attempt to fill the preferences property no matter the status.  Then the test could create mock prefs and ensure they're in the snapshot, similar to what the modifiedPreferences test does.

Comment 24

[Allison Naaktgeboren :ally]

Attachment: v5 part 1/1

Comment 25

[Allison Naaktgeboren :ally]

(In reply to Drew Willcoxon :adw from comment #23)
> Well, to be clear it's not that I think it couldn't be done, it's that I
> didn't think it's worth testing.  But I gave it a second thought after Ally
> and I talked, and I think it is worth testing because it could be the case
> that the data provider is actually not capturing services.sync.* prefs at
> all, and if there's no test we'd never know.
> 
> So what I'd do, for whatever it's worth, is have status and preferences
> properties like Blair suggests, and have the data provider attempt to fill
> the preferences property no matter the status.  Then the test could create
> mock prefs and ensure they're in the snapshot, similar to what the
> modifiedPreferences test does.

that still won't fool the check for sync being enabled, which is not pref based:
if (main.Weave.Status.checkSetup() != main.Weave.CLIENT_NOT_CONFIGURED) 

So even if I monkeyset prefs in the test as done with modifiedPreferences, the data provider would correctly not provide them to me.

Comment 26

[Gregory Szorc [:gps]]

Please go through the Sync API to set up a dummy Sync account during tests. If implemented on mozilla-central today:

  Weave.Service.identity.account = "[e-mail]";
  Weave.Service.identity.basicPassword = "password";
  Weave.Service.identity.syncKey = Weave.Utils.generatePassphrase();

Or, if on services-central, we now export a testing-only JS module with a convenience API:

  Cu.import("resource://testing-common/services/sync/utils.js");
  setBasicCredentials("[e-mail]", "password", "Weave.Utils.generatePassphrase());

Do *NOT* go through the prefs API and try to set up Sync in about:support code, test or otherwise. Sync has an API. Sync should be a black box to about:support. The fact we use prefs under the hood should be invisible to everyone except Sync.

Comment 27

[Gregory Szorc [:gps]]

Per hallway convo with ally, what you are looking for is: Weave.Service.engines.getEnabled()

Comment 28

[Gregory Szorc [:gps]]

After actually looking at the patch, my conclusion is the architecture of about:support is reversed from where it should be. You are exposing the core logic of features/services in Troubleshoot.jsm. Instead, what you should be doing is exposing some kind of API/hook in Troubleshoot.jsm. Features/services (like Sync) should register with it when they initialize. When Troubleshoot.jsm needs to present about:support, it calls out into each service and says "tell me about your current state." Troubleshoot.jsm collects the results and presents something to the user.

This is a superior architecture because the core logic for how services work is contained within the implementation of the service itself and is not fragmented by Troubleshoot.jsm. As things are currently implemented, you are asking me to surrender control of Sync's API to Troubleshoot.jsm. If I change an internal implementation detail of Sync, now I need to know that I need to ensure Troubleshoot.jsm isn't relying on that. Instead, if the API for "what is your state" lived in Sync, all that code would be self-contained and Sync and Troubleshoot.jsm could happily live in their loosely coupled worlds, not having to worry about what the other is doing.

So, what I'd like to see is for Troubleshoot.jsm to expose a "service" that will be consumed like so:

  Cu.import("resource://gre/toolkit/content/troubleshoot.jsm");

  Troubleshoot.registerService("Sync", function onStateQuery(cb) {
    cb({
      modifiedPrefs: {
        foo: "bar",
      },
    });
  });

This allows me (or anyone else, including random extensions) to opt in to about:support without having to modify Troubleshoot.jsm. Basically, I want Troubleshoot.jsm to expose an API for adding entities to dataProviders. That API could even be exporting that symbol from the module!

Comment 29

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #28)

> So, what I'd like to see is for Troubleshoot.jsm to expose a "service" that
> will be consumed like so:

Two opposable thumbs up.

Comment 30

[Mike Connor [:mconnor]]

I think we're getting squarely into followup territory.  I'm going to posit the following:

* We're already well past the point of diminishing returns for time spent vs. value, given the massive bitrotting midway through
* The failure case here is minimally bad (we won't display information that can be retrieved in another way)
* There's a bigger discussion to be had about whether the refactoring gps proposed is worth it, and we can revisit testability as a part of that process.

Given those, can we land things as-is, and consider how/if to expand testing of this part as part of the followup implied in the third bullet and comment 28 ?

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 665153 [details] [diff] [review]
v5 part 1/1

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

What mconnor said.

Just one final fixup for the text copying:

::: toolkit/content/aboutSupport.js
@@ +190,5 @@
> +      }
> +      $.append($("sync-tbody"), trs);
> +    }
> +    else {
> +      $("syncNotSupported").hidden = false;

Unfortunately, we have a long-standing (read: ancient) platform bug where hidden nodes get put into the clipboard just like visible nodes do. So hitting "Copy text to clipboard" when Sync *is* setup results in "Service is not enabled" still being in the copied text. 

So rather than hiding, you'll need to just remove the syncNotSupported table from the DOM. And when Sync isn't enabled, showing the Name/Value headers doesn't make sense, so remove that table from the DOM.

::: toolkit/content/aboutSupport.xhtml
@@ +260,5 @@
> +        <tbody id="sync-tbody">
> +        </tbody>
> +      </table>
> +      <table>
> +        <td id="syncNotSupported" hidden="true">

This id should be on the <table>

Comment 32

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #31)
> > +        <td id="syncNotSupported" hidden="true">
> 
> This id should be on the <table>

Er, unless you end up re-using that table for bug 792133.

Comment 33

[Allison Naaktgeboren :ally]

(In reply to Blair McBride (:Unfocused) from comment #32)
> (In reply to Blair McBride (:Unfocused) from comment #31)
> > > +        <td id="syncNotSupported" hidden="true">
> > 
> > This id should be on the <table>
> 
> Er, unless you end up re-using that table for bug 792133.

I was, until :mconnor said he wanted the button in the title, so I guess I don't.

Just out of curiosity, what is the # of this ancient platform bug?

Comment 34

[Allison Naaktgeboren :ally]

Attachment: v6 part 1/1

Comment 35

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7e167eb377

Thank you Unfocused & adw

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f7e167eb377

Comment 37

[Tracy Walker [:tracy]]

Looks good with nightly m-c build of 20121004

Comment 39

[Tony Mechelynck [:tonymec]]

Circumstantial evidence (last good, first bad, what is there between them that touches the file with XML parsing error) seems to point to this fix as the cause for SeaMonkey regression bug 798584 but I can't understand how.

Comment 40

[Philip Chee]

https://hg.mozilla.org/mozilla-central/rev/2f7e167eb377#l3.12
>   3.12 +  <!ENTITY % syncBrandDTD SYSTEM "chrome://browser/locale/syncBrand.dtd"> %syncBrandDTD;

https://hg.mozilla.org/mozilla-central/rev/2f7e167eb377#l3.30
>    3.30 +        &syncBrand.fullName.label;

Toolkit should not use any strings from /browser/ as this will break about:support for SeaMonkey, etc. For example mobile/android/puts this entity in sync_strings.dtd

Comment 41

[Dave Townsend [:mossop]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1fc99346b6

Please don't reland this without the changes from bug 798160.

Comment 42

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, duh - what Philip said. Guess we have to ignore the Sync branding here :\ Which means just adding an entity in aboutSupport.dtd with the string "Sync", since we can't put product names in Toolkit.

And of course everything else should be wrapped in a #ifdef MOZ_SERVICES_SYNC too.

Comment 43

[Allison Naaktgeboren :ally]

also the powers that be would like the username & account name removed. see bug 798160

Comment 44

[Allison Naaktgeboren :ally]

Attachment: the redux - or fixing fallout from first checkin

- removes user account details
- has #ifdef wrappers to accommodate disabling sync at compile time
- have fixed strings to not break seamonkey & co (along with a comment explaining why and what it should match for localizers)
-- have not come up with a better answer to disabling the snippet of the json definition when sync is disabled at compile time. All methods I have explored disable function calls or files. :/

Comment 45

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 672881 [details] [diff] [review]
the redux - or fixing fallout from first checkin

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

::: toolkit/content/tests/browser/browser_Troubleshoot.js
@@ +131,5 @@
>      },
> +#ifdef MOZ_SERVICES_SYNC
> +    syncService: {
> +      required: true,
> +      type: "object",

Yea, turns out we don't have a nice way to make browser-chrome tests go through the preprocessor... so no #ifdef here.

But I think it should be good enough to set required=false for this, and add "prefs" and "isEnabled" as required sub-properties. ie:

syncService: {
  required: false,
  type: "object",
  properties: {
    prefs: {
      required: true,
      type: "object"
    },
    isEnabled: {
      required: true,
      type: "boolean"
    }
  }
}

Comment 46

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 47

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.