1339811 - Align troubleshooting information (about:support) with M-C

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Richard Marti (:Paenglab)]

Today TB does the about:support things by it self but relies on M-C strings for most of the parts. When M-C makes some updates this often breaks TB because the strings aren't correct.

Also do we not profit from improvements on M-C side.

TB should use as much M-C modules as it can and only maintain the mail modules.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

It seems, I learned a bit in programming since last time I tried to use the toolkit about:support and include the mail part. This time everything works.

What does this patch?
It copies the aboutSupport.xhtnl and aboutSupport.js from toolkit to mail and adds to them the accounts part and removes the experiments part.

I removed all unneeded files except accounts.js for the accounts part and export.js for the send via email as this packs it nicely into html.

I also removed the overlay files and moved their pieces into the normally used files.

The mail aboutSupport.css file was placed in windows and during building copied to Linux and macOS. I moved it to shared because it is a shared file.

I had to add a pref (security.sandbox.content.level) for the sandbox which without shows this error:

Troubleshoot data provider failed: sandbox
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/Troubleshoot.jsm :: sandbox :: line 582"  data: no]  Troubleshoot.jsm:162

I could also hide/remove the sandbox part on the page as it shows nothing.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8838889 [details] [diff] [review]
aboutSupport.patch

Aren't you luck I can build again ;-)

This looks nice. Clicking on "Copy text to clipboard" gives:
JavaScript error: chrome://messenger/content/about-support/export.js, line 193:
ReferenceError: getCrashesText is not defined

I can see Sandbox at the bottom.

Graphics doesn't look 100% good:
Old: WebGL 1 Renderer	Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D9Ex vs_3_0 ps_3_0) -- OpenGL ES 2.0 (ANGLE 2.1.0.2a250c8a0e15)
New: WebGL 1 Renderer	(no creation error info)
Firefox:
WebGL Renderer	Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D9Ex vs_3_0 ps_3_0)
WebGL2 Renderer	WebGL creation failed: * Error during ANGLE OpenGL init. * Error during ANGLE OpenGL init. * Error during ANGLE OpenGL init. * Error during ANGLE OpenGL init. * Error during ANGLE OpenGL init. * Exhausted GL driver caps.
Note that FF has the WebGL2 stuff all in the one line, it TB it's multiple lines.

Also, does
mozmake SOLO_TEST=content-tabs/test-about-support.js mozmill-one
(run in the object directory) still pass?

Aceman is working on enhancing the test.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: aboutExport.patch v2

(In reply to Jorg K (GMT+1) from comment #2)
> Comment on attachment 8838889 [details] [diff] [review]
> aboutSupport.patch
> 
> This looks nice. Clicking on "Copy text to clipboard" gives:
> JavaScript error: chrome://messenger/content/about-support/export.js, line
> 193:
> ReferenceError: getCrashesText is not defined

Fixed

> I can see Sandbox at the bottom.

Me too but it's only a line, which isn't very informative. Or is someone planning to activate the sandbox on TB?

> Graphics doesn't look 100% good:
> Old: WebGL 1 Renderer	Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D9Ex
> vs_3_0 ps_3_0) -- OpenGL ES 2.0 (ANGLE 2.1.0.2a250c8a0e15)
> New: WebGL 1 Renderer	(no creation error info)
> Firefox:
> WebGL Renderer	Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D9Ex vs_3_0
> ps_3_0)
> WebGL2 Renderer	WebGL creation failed: * Error during ANGLE OpenGL init. *
> Error during ANGLE OpenGL init. * Error during ANGLE OpenGL init. * Error
> during ANGLE OpenGL init. * Error during ANGLE OpenGL init. * Exhausted GL
> driver caps.
> Note that FF has the WebGL2 stuff all in the one line, it TB it's multiple
> lines.

I had this too with one profile and this looks like a issue by Troubleshoot.jsm. I think we need to check with what settings this happens and then let fix it by toolkit.

> Also, does
> mozmake SOLO_TEST=content-tabs/test-about-support.js mozmill-one
> (run in the object directory) still pass?

I have tests disabled, please can you check this?

> Aceman is working on enhancing the test.

Aceman, can you test this too? Maybe it needs some tweaks to work with the new aboutSupport.

Comment 4

[:aceman]

Oh nice cleanup :)

Does it mean we will still need to sync aboutSupport.js/.xhtml from time to time?

I also see an empty Sandbox section at the bottom. Do we not support those features? Could we show 'false' for the items there instead of empty table?

Yes, that empty table would fail the new test but I could add it to the exceptions.

I do not see the error on clicking "Copy text to clipboard" or "Send via email".

The test content-tabs/test-about-support.js also fails due to "Timeout waiting for about:support's gExtensions to populate.". Maybe we just need a different check, because the Extensions table seems fine.

Comment 5

[:aceman]

Yes I can can update the test when you finish the main patch. Thanks.

Comment 6

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #4)
> Oh nice cleanup :)
> 
> Does it mean we will still need to sync aboutSupport.js/.xhtml from time to
> time?

Yes, but only this two. All other changes in Troubleshoot.jsm with their string changes should automatically work for us then.

> I also see an empty Sandbox section at the bottom. Do we not support those
> features? Could we show 'false' for the items there instead of empty table?

This would need a change in Troubleshoot.jsm as this creates the content.

> Yes, that empty table would fail the new test but I could add it to the
> exceptions.
> 
> I do not see the error on clicking "Copy text to clipboard" or "Send via
> email".

This is fixed in the second patch.

> The test content-tabs/test-about-support.js also fails due to "Timeout
> waiting for about:support's gExtensions to populate.". Maybe we just need a
> different check, because the Extensions table seems fine.

gExtensions is no more used extensions.js is removed and done by Troubleshoot.jsm.

(In reply to :aceman from comment #5)
> Yes I can can update the test when you finish the main patch. Thanks.

Thank you, let's see what Jörg means about my second patch.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #3)
> Me too but it's only a line, which isn't very informative. Or is someone
> planning to activate the sandbox on TB?
Well, a heading without content is ugly. You said:
"I could also hide/remove the sandbox part on the page as it shows nothing."
Please do ;-) Or simply show a table saying "Off" or whatever.

> > Graphics doesn't look 100% good:
> > Old: WebGL 1 Rendere Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D9Ex ...
> > New: WebGL 1 Renderer	(no creation error info)
> I had this too with one profile and this looks like a issue by
> Troubleshoot.jsm. I think we need to check with what settings this happens
> and then let fix it by toolkit.
I think we need to fix this before landing this. TB 54 Daily shows the right information and so does FF 51. I haven't tried a FF Nightly.

(no creation error info) comes from Troubleshoot.jsm (as you said). I think you'll need to investigate this a bit.

Comment 8

[:aceman]

(In reply to Jorg K (GMT+1) from comment #7)
> (In reply to Richard Marti (:Paenglab) from comment #3)
> > Me too but it's only a line, which isn't very informative. Or is someone
> > planning to activate the sandbox on TB?
> Well, a heading without content is ugly. You said:
> "I could also hide/remove the sandbox part on the page as it shows nothing."
> Please do ;-) Or simply show a table saying "Off" or whatever.

Yes, I'd also like it to show 'false' for all the features if they are not enabled in TB (they are in FF54). But if we lack some support for even reporting the state (or we must have AppConstants.MOZ_SANDBOX turned off) then we can just hide the section via css.

Comment 9

[:aceman]

OK, I am updating the test.
It already found a problem: checking "show account names" does not show the profile directory path.

Comment 10

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #7)
> (In reply to Richard Marti (:Paenglab) from comment #3)
> > Me too but it's only a line, which isn't very informative. Or is someone
> > planning to activate the sandbox on TB?
> Well, a heading without content is ugly. You said:
> "I could also hide/remove the sandbox part on the page as it shows nothing."
> Please do ;-) Or simply show a table saying "Off" or whatever.
> 
> > > Graphics doesn't look 100% good:
> > > Old: WebGL 1 Rendere Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D9Ex ...
> > > New: WebGL 1 Renderer	(no creation error info)
> > I had this too with one profile and this looks like a issue by
> > Troubleshoot.jsm. I think we need to check with what settings this happens
> > and then let fix it by toolkit.
> I think we need to fix this before landing this. TB 54 Daily shows the right
> information and so does FF 51. I haven't tried a FF Nightly.

I can reproduce this in FX with set layers.acceleration.disabled to true -> bug in toolkit. I'll file a bug.

(In reply to :aceman from comment #9)
> OK, I am updating the test.
> It already found a problem: checking "show account names" does not show the
> profile directory path.

This doesn't exist in toolkit's aboutSupport.

Comment 11

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #10)
> (In reply to :aceman from comment #9)
> > It already found a problem: checking "show account names" does not show the
> > profile directory path.
> This doesn't exist in toolkit's aboutSupport.

Yes, so we need to decide if we can loose this feature.

I can update the test to not check this (check other private data).

Comment 12

[Richard Marti (:Paenglab)]

Filed bug 1340858 for the WebGL error.

Comment 13

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #11)
> (In reply to Richard Marti (:Paenglab) from comment #10)
> > (In reply to :aceman from comment #9)
> > > It already found a problem: checking "show account names" does not show the
> > > profile directory path.
> > This doesn't exist in toolkit's aboutSupport.
> 
> Yes, so we need to decide if we can loose this feature.

I see no need for this as the button to open the profile works. If someone then needs the path he can copy it from file browser.

And as I see it, this would need again the aboutSupportMac.js, aboutSupportUnix.js and aboutSupportWin32.js files.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch v3

The sandbox part is now commented out. I think adding a special table which says, sandbox is disabled, makes some users asking for this feature. And with our limited development we can't implement this.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #12)
> Filed bug 1340858 for the WebGL error.
Yes, but for the WebGL 2 renderer when I was complaining about the WebGL 1 display.

Comment 16

[Jorg K (CEST = GMT+2)]

Trying the new patch:
With HWA:
WebGL 1 Renderer - (no creation error info)
WebGL 2 Renderer - Google Inc. -- ANGLE (AMD Radeon HD 7660D Direct3D11 vs_5_0 ps_5_0)

Without HWA:
WebGL 1 Renderer - (no creation error info)
WebGL 2 Renderer - WebGL creation failed: ... (various errors).

I think we're doing something wrong to WebGL 1 comes out wrong since that works in FF.

Comment 17

[Richard Marti (:Paenglab)]

I See the same error on FX with layers.acceleration.disabled=true. That's also why I filed bug 1340858.

Comment 18

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #13)
> I see no need for this as the button to open the profile works. If someone
> then needs the path he can copy it from file browser.

The button does not always work on Linux. (Therefore we also show the path in the remove account dialog.)
Strangely, it works for me in Firefox. nsIFile.reveal() fails in TB, works in FF. Yeah, we could either fix this function, or implement the displaying of path in Toolkit.

(In reply to Richard Marti (:Paenglab) from comment #14)
> The sandbox part is now commented out. I think adding a special table which
> says, sandbox is disabled, makes some users asking for this feature. And
> with our limited development we can't implement this.

Is this the single difference against the toolkit version of the file? Could we hide the section via css so that we can just copy over a new version of the xhtml any time later without forgetting we need to remove the sandbox part? But looking at the toolkit file, they already have the #if defined(MOZ_SANDBOX) there. So what is the change needed in our version to hide the section?

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #17)
> I see the same error on FX with layers.acceleration.disabled=true. That's
> also why I filed bug 1340858.
Sure, for WebGL 2. But for me, WebGL 1 is always populated in FF (Nightly) when it's always in error in TB with your patch, see comment #16. And the error in WebGL 2 is expected and handled by a try/catch in JS (which also shows in the console). So IMHO bug 1340858 is invalid and we need to look into WebGL 1 in this bug here.

Comment 20

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #18)
> (In reply to Richard Marti (:Paenglab) from comment #13)
> > I see no need for this as the button to open the profile works. If someone
> > then needs the path he can copy it from file browser.
> 
> The button does not always work on Linux. (Therefore we also show the path
> in the remove account dialog.)
> Strangely, it works for me in Firefox. nsIFile.reveal() fails in TB, works
> in FF. Yeah, we could either fix this function, or implement the displaying
> of path in Toolkit.

Also with this patch applied?

> (In reply to Richard Marti (:Paenglab) from comment #14)
> > The sandbox part is now commented out. I think adding a special table which
> > says, sandbox is disabled, makes some users asking for this feature. And
> > with our limited development we can't implement this.
> 
> Is this the single difference against the toolkit version of the file? Could
> we hide the section via css so that we can just copy over a new version of
> the xhtml any time later without forgetting we need to remove the sandbox
> part? But looking at the toolkit file, they already have the #if
> defined(MOZ_SANDBOX) there. So what is the change needed in our version to
> hide the section?

No, I added the Send email button, the checkmark to show the account names and the accounts part. And I removed the experiments part. Also some links to our files (js and CSS) are added.

But I think the most important thing is, we use Troubleshoot.jsm and become automatically the enhancements in the tables if they add some.

Comment 21

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #19)
> (In reply to Richard Marti (:Paenglab) from comment #17)
> > I see the same error on FX with layers.acceleration.disabled=true. That's
> > also why I filed bug 1340858.
> Sure, for WebGL 2. But for me, WebGL 1 is always populated in FF (Nightly)
> when it's always in error in TB with your patch, see comment #16. And the
> error in WebGL 2 is expected and handled by a try/catch in JS (which also
> shows in the console). So IMHO bug 1340858 is invalid and we need to look
> into WebGL 1 in this bug here.

I think Troubleshoot.jsm uses the system functions and TB is doing something wrong. Maybe this is wishful thinking as I have no  idea how this all works.

And maybe someone needs to correlate how the old TB about:support code and Troubleshoot.jsm do check the WebGL values.

Comment 22

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #20)
> > The button does not always work on Linux. (Therefore we also show the path
> > in the remove account dialog.)
> > Strangely, it works for me in Firefox. nsIFile.reveal() fails in TB, works
> > in FF. Yeah, we could either fix this function, or implement the displaying
> > of path in Toolkit.
> Also with this patch applied?

Yes, but I found out it may again be because I have some GIO service compiled out...

 
> > Is this the single difference against the toolkit version of the file? Could
> > we hide the section via css so that we can just copy over a new version of
> > the xhtml any time later without forgetting we need to remove the sandbox
> > part? But looking at the toolkit file, they already have the #if
> > defined(MOZ_SANDBOX) there. So what is the change needed in our version to
> > hide the section?
> No, I added the Send email button, the checkmark to show the account names
> and the accounts part. And I removed the experiments part. Also some links
> to our files (js and CSS) are added.

How did you remove the experiments part? And how do we document all the changes done so that they can be re-done when the file is synced with toolkit again?

> But I think the most important thing is, we use Troubleshoot.jsm and become
> automatically the enhancements in the tables if they add some.

Yes, but how do you hide the Sandbox table?

Comment 23

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #22)
> (In reply to Richard Marti (:Paenglab) from comment #20)
> > > Is this the single difference against the toolkit version of the file? Could
> > > we hide the section via css so that we can just copy over a new version of
> > > the xhtml any time later without forgetting we need to remove the sandbox
> > > part? But looking at the toolkit file, they already have the #if
> > > defined(MOZ_SANDBOX) there. So what is the change needed in our version to
> > > hide the section?
> > No, I added the Send email button, the checkmark to show the account names
> > and the accounts part. And I removed the experiments part. Also some links
> > to our files (js and CSS) are added.
> 
> How did you remove the experiments part? And how do we document all the
> changes done so that they can be re-done when the file is synced with
> toolkit again?

I removed it totally, but can apply it again and hide it through commenting out in xhtml file.
I can also comment all additions/changes.

> > But I think the most important thing is, we use Troubleshoot.jsm and become
> > automatically the enhancements in the tables if they add some.
> 
> Yes, but how do you hide the Sandbox table?

Through commenting out in xhtml file:
<!-- Not yet used. TB does not support the sandbox
code...
-->

Comment 24

[Jorg K (CEST = GMT+2)]

Sotaro-san, sorry for NI'ing you on two bugs with a similar problem. Here's what we try to do.

We copied aboutSupport.js to C-C and made minimal changes to it to include mail account information on the about:support page.

Sadly the Webgl1 information is not properly displayed in TB and we can't work out why. I've done a little debugging in Troubleshoot.jsm like so:

        let gl = null;
        try {
            dump("=== Try "+contextType+"\n");
            gl = canvas.getContext(contextType);
        } catch (e) {
            dump("=== Catch "+contextType+"\n");
            if (!creationError) {
                creationError = e.toString();
            }
        }
        if (!gl) {
            data[keyPrefix + "Renderer"] = creationError || "(no creation error info)";
            return;
        }

I get this debug:
=== Try webgl
=== Try webgl2

So apparently there was no error creating the canvas element. With and without HWA enabled, all the information for Webgl2 is appropriately displayed (see comment #16), but the Webgl information isn't properly displayed. Since we see "(no creation error info)", creationError must have been null, so no error, and gl must have been returned null from |gl = canvas.getContext(contextType);| where contextType=="webgl".

Since we're not experts in any of this, we need help. Can you please give us some hints.

Comment 25

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch v4

The same patch with all changes commented in aboutSupport.js|xhtml

Comment 26

[:aceman]

Comment on attachment 8838964 [details] [diff] [review]
aboutSupport.patch v4

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

::: mail/app/profile/all-thunderbird.js
@@ +865,5 @@
>  
>  // calendar promotion status
>  pref("mail.calendar-integration.opt-out", false);
> +
> +#if defined(MOZ_CONTENT_SANDBOX)

Can you use #ifdef ?

::: mail/base/content/mailCore.js
@@ +598,5 @@
> +    let tabmail = document.getElementById("tabmail");
> +    tabmail.openTab("contentTab",
> +                    {contentPage: "about:support",
> +                     clickHandler: "specialTabs.aboutClickHandler(event);" });
> +  }

All the lines are strangely indented.

::: mail/components/about-support/content/aboutSupport.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +var { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Please add a comment here at the top that this is a copy of mozilla/toolkit/content/aboutSupport.js with modifications for TB.

@@ +593,5 @@
> +    // Clear the no-copy class
> +    $("prefs-user-js-section").className = "";
> +  },
> +/* Not used by TB
> +  sandbox: function sandbox(data) {

Aha, this is why the table is empty. When I enable this, the table gets populated on Linux. All sandbox features show a value of true. On other platforms only the row with sandbox level (of 0) should be shown.

So why do we hide this?

::: mail/components/about-support/content/aboutSupport.xhtml
@@ +628,5 @@
> +        </tbody>
> +      </table>
> +      -->
> +      <!-- - - - - - - - - - - - - - - - - - - - - -->
> +#if defined(MOZ_SANDBOX)

If have found out we have AppConstants.MOZ_SANDBOX==true. So then I wonder why the table stays unpopulated. It seems only linux has any values there (and I see those on FF).
The code is:
  if (AppConstants.platform == "linux") {
      const keys = ["hasSeccompBPF", "hasSeccompTSync",
                    "hasPrivilegedUserNamespaces", "hasUserNamespaces",
                    "canSandboxContent", "canSandboxMedia"];
    
      let sysInfo = Cc["@mozilla.org/system-info;1"].
                    getService(Ci.nsIPropertyBag2);
      for (let key of keys) {
        if (sysInfo.hasKey(key)) {
          data[key] = sysInfo.getPropertyAsBool(key);
        }
      }
    }

::: mail/components/about-support/content/export.js
@@ +238,5 @@
>    textFragmentAccumulator.push(text);
>  }
> +
> +/**
> + * Returns a plaintext representation of extension data.

Does this comment match the function code? The function seems to be about crashes, not extensions. How does toolkit get away with not having this function?

::: mail/components/about-support/content/init.js
@@ -47,5 @@
> -      })]);
> -
> -  let fsType;
> -  try {
> -    fsType = AboutSupport.getFileSystemType(currProfD);

It is a pity this is going away. The filesystem type is important information for us. Could the aboutSupportPlatform.js files be preserved and this information tacked onto some toolkit generated field, from our code (in our modified aboutSupport.js)?

Comment 27

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch v5

(In reply to :aceman from comment #26)
> Comment on attachment 8838964 [details] [diff] [review]
> aboutSupport.patch v4
> 
> Review of attachment 8838964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/app/profile/all-thunderbird.js
> @@ +865,5 @@
> >  
> >  // calendar promotion status
> >  pref("mail.calendar-integration.opt-out", false);
> > +
> > +#if defined(MOZ_CONTENT_SANDBOX)
> 
> Can you use #ifdef ?

Fixed, it was a direct copy from FX

> ::: mail/base/content/mailCore.js
> @@ +598,5 @@
> > +    let tabmail = document.getElementById("tabmail");
> > +    tabmail.openTab("contentTab",
> > +                    {contentPage: "about:support",
> > +                     clickHandler: "specialTabs.aboutClickHandler(event);" });
> > +  }
> 
> All the lines are strangely indented.

Fixed

> ::: mail/components/about-support/content/aboutSupport.js
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +var { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> 
> Please add a comment here at the top that this is a copy of
> mozilla/toolkit/content/aboutSupport.js with modifications for TB.

Done

> @@ +593,5 @@
> > +    // Clear the no-copy class
> > +    $("prefs-user-js-section").className = "";
> > +  },
> > +/* Not used by TB
> > +  sandbox: function sandbox(data) {
> 
> Aha, this is why the table is empty. When I enable this, the table gets
> populated on Linux. All sandbox features show a value of true. On other
> platforms only the row with sandbox level (of 0) should be shown.
> 
> So why do we hide this?

The table was empty, only a line was drawn. But probably because I added the security.sandbox.content.level pref it works -> now unhidden

> ::: mail/components/about-support/content/export.js
> @@ +238,5 @@
> >    textFragmentAccumulator.push(text);
> >  }
> > +
> > +/**
> > + * Returns a plaintext representation of extension data.
> 
> Does this comment match the function code? The function seems to be about
> crashes, not extensions. How does toolkit get away with not having this
> function?

It was a direct copy of the deleted crashes.js. I exchanged "extension" with "crashes".

> ::: mail/components/about-support/content/init.js
> @@ -47,5 @@
> > -      })]);
> > -
> > -  let fsType;
> > -  try {
> > -    fsType = AboutSupport.getFileSystemType(currProfD);
> 
> It is a pity this is going away. The filesystem type is important
> information for us. Could the aboutSupportPlatform.js files be preserved and
> this information tacked onto some toolkit generated field, from our code (in
> our modified aboutSupport.js)?

Feel free to add it. I don't know what all is needed to make it work.

Comment 28

[:aceman]

Attachment: aboutSupport.patch v6

OK, I added back the FS type detection and also the display of the profile path. As we have a copy of the code populating some of the fields in the basic application info table, I could tack all this into it.
I'm not sure clicking the profile path opens it in explorer (as clicking the button would). Please test that. Or it can be a normal text, as the button is still there.

Comment 29

[:aceman]

Attachment: test

This updates the test for the new state of the page. I changed it to not check the profile path as I thought the feature is going away. But now it seems we can keep it easily. But it is probably irrelevant which element we check in the test as private, as long as it has the data-private class.

Try run (with patch v5, but with v6 is passes locally on linux too): https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c410850c4767b904c8a6ebd5472b8b46b3ca586d

Comment 30

[Jorg K (CEST = GMT+2)]

Could you add the test from bug 1339436 here as well?

Comment 31

[:aceman]

Sadly that one is still not reliable using the appmenu (see https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cf7cd27228ca351a472b8d11f87239078743616d). The test patch here is to be applied without the one in bug 1339436.

But if you mean to sneak the empty-table-checking logic into this one, I can do that ;) It just does not really have to do anything with the change here.

Comment 32

[Richard Marti (:Paenglab)]

Comment on attachment 8838984 [details] [diff] [review]
aboutSupport.patch v6

Yes, it opens in Explorer and on macOS in Finder. Thank you very much.

Comment 33

[:aceman]

Do we need anything more here?
Yes, the GPU section bug, but do we need to solve it here? GPU acceleration is not very useful in TB, AFAIK it is just enabled to be on par with FF (meaning have the same bugs:)).

I hope we didn't loose any more feature than the ones I have spotted (and restored).
Is this ready for review?

Comment 34

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #33)
> Yes, the GPU section bug, but do we need to solve it here?
I'm opposed to landing anything that breaks something what works currently (the GPU section). There is NI for someone who should know, so we just need to wait.

Comment 35

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #33)
> I hope we didn't loose any more feature than the ones I have spotted (and
> restored).

I don't know of an other feature we will loose.

Comment 36

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch v7

First improvement we can now easily port just landed: bug 1286865 - Expose rejected syscall log in about:support. Linux only, but a first step. :-)

Comment 37

[Jorg K (CEST = GMT+2)]

Jeff, looking at blame on Troubleshoot.jsm I saw that you did the WebGl part. Could you look at my comment #24 and help us here. Thank you in advance.

Comment 38

[Jorg K (CEST = GMT+2)]

I should have said "please": Could you please look at my comment #24 and help us here.

Comment 39

[Kelsey Gilbert [:jgilbert]]

(In reply to Jorg K (GMT+1) from comment #37)
> Jeff, looking at blame on Troubleshoot.jsm I saw that you did the WebGl
> part. Could you look at my comment #24 and help us here. Thank you in
> advance.

Is WebGL supposed to be enabled on c-c? It looks like you used to use a completely different path to check webgl renderer info, without actually touching webgl:
> webglrenderer = gfxInfo.getWebGLParameter("full-renderer");

Indeed, this shows different values than in Firefox, where we request the RENDERER and VENDOR from WebGL directly.

If WebGL is deactivated/disabled on c-c, the old method for querying could return info, but the new method (the one we use in Firefox) could quietly fail to create a context.

Comment 40

[Jorg K (CEST = GMT+2)]

(In reply to Jeff Gilbert [:jgilbert] from comment #39)
> If WebGL is deactivated/disabled on c-c, the old method for querying could
> return info, but the new method (the one we use in Firefox) could quietly
> fail to create a context.
Thanks for the reply.

Our problem is that the Thunderbird developers are a very small group that has to know everything about Thunderbird and mailnews internals and we also have to interface with many corners of M-C code, ranging from Core::Editor, Data/Time/ICU, docshell, content policies, toolkit spelling, toolkit autocomplete, security, certificates, installer, accessibility, just to name a few quickly. It's very hard, or let's say, impossible, to know all these things and to speak to Mozilla core developers on an equal level. So you just have to bear with our ignorance or (dangerous) half knowledge.

That said, yes, you are right, |webglrenderer = gfxInfo.getWebGLParameter("full-renderer");| was previously used to retrieve some WebGL information. I had to look up WebGL on Google (JavaScript API for rendering 3D graphics) to find out what we're talking about.

So what could be the way forward here? In general we'd like to clone aboutSupport.js that uses Troubleshoot.jsm. When we do, we get the same output for WebGL 2 as in Firefox (comment #16). So is there any way to get something useful for WebGL 1 as well? How would we find out whether WebGL is enabled? How would we enable it and what (dis)advantages would that have? Or should we just replace the graphics section in aboutSupport.js with the old code we had in Thunderbird?

Comment 41

[Kelsey Gilbert [:jgilbert]]

I suspect the issue is Thunderbird not building with MOZ_WEBGL_CONFORMANT=1:
https://dxr.mozilla.org/mozilla-central/rev/d0462b0948e0b1147dcce615bddcc46379bdadb2/browser/confvars.sh#58
https://dxr.mozilla.org/mozilla-central/rev/d0462b0948e0b1147dcce615bddcc46379bdadb2/mobile/android/confvars.sh#52

I'm removing this old flag from Gecko in bug 1341459.

Comment 42

[Jorg K (CEST = GMT+2)]

Thanks!

Richard, please add
MOZ_WEBGL_CONFORMANT=1
to mail/confvars.sh

MOZ_PROFILE_MIGRATOR=1
MOZ_WEBGL_CONFORMANT=1 <---
MOZ_JSDOWNLOADS=1

to see whether that fixes the issue.

Comment 43

[Richard Marti (:Paenglab)]

Attachment: aboutConfig.patch v8 (the final one?)

Thank you Jeff.

Adding the variable did the trick. I follow bug 1341459 and will remove the variable after landing of it.

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 8839819 [details] [diff] [review]
aboutConfig.patch v8 (the final one?)

I'm happy with anything that works and will be better maintainable in the future. I haven't tested the latest patch since the confvars.sh change will trigger a complete rebuild.

Let's have Aceman's approval here, since he had some previous comments.

Also, we need to review/rubberstamp his test changes, and I'd like to see the snipped from bug 1339436 added (sneak the empty-table-checking logic into this one).

Comment 45

[:aceman]

Comment on attachment 8839819 [details] [diff] [review]
aboutConfig.patch v8 (the final one?)

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

OK almost done. This works for for me on Linux, even dumping all the verbose WebGL info (OpenGL extensions) and the new syscall table.
BUT there seems to be a problem on Windows (https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fc9c4a250d0600dbd884133e9796959b35c15198).
The test has uncovered there are some fields on the page containing "null" or "undefined".
Maybe it is due to the build servers having low OpenGL support:
17:40:10     INFO -  WebGL(17B54000)::ForceLoseContext
17:40:10     INFO -  JavaScript warning: resource://gre/modules/Troubleshoot.jsm, line 448: Error: WebGL warning: Failed to create WebGL context: WebGL creation failed:
17:40:10     INFO -  * Error during ANGLE OpenGL init.
17:40:10     INFO -  * Error during ANGLE OpenGL init.
17:40:10     INFO -  * Error during ANGLE OpenGL init.
17:40:10     INFO -  * Error during ANGLE OpenGL init.
17:40:10     INFO -  * Error during ANGLE OpenGL init.
17:40:10     INFO -  * Exhausted GL driver caps.

Please see if you can reproduce this on local Windows.

Then, both Windows and OS X show "JavaScript error: chrome://messenger/content/about-support/aboutSupport.js, line 665: TypeError: data.syscallLog is undefined". It seems that block wasn't made conditional on being on Linux (as the table in xthml file is). Maybe a bug in toolkit (the table is new).

Comment 46

[:aceman]

Attachment: test v2

Updated test. Put back checking of the profile path (in a reduced form) and skips the syscall table which is usually empty.
Shows the "null" field problems on Windows: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fc9c4a250d0600dbd884133e9796959b35c15198

Comment 47

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #45)
> Comment on attachment 8839819 [details] [diff] [review]
> aboutConfig.patch v8 (the final one?)
> 
> Review of attachment 8839819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK almost done. This works for for me on Linux, even dumping all the verbose
> WebGL info (OpenGL extensions) and the new syscall table.
> BUT there seems to be a problem on Windows
> (https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=fc9c4a250d0600dbd884133e9796959b35c15198).
> The test has uncovered there are some fields on the page containing "null"
> or "undefined".
> Maybe it is due to the build servers having low OpenGL support:
> 17:40:10     INFO -  WebGL(17B54000)::ForceLoseContext
> 17:40:10     INFO -  JavaScript warning:
> resource://gre/modules/Troubleshoot.jsm, line 448: Error: WebGL warning:
> Failed to create WebGL context: WebGL creation failed:
> 17:40:10     INFO -  * Error during ANGLE OpenGL init.
> 17:40:10     INFO -  * Error during ANGLE OpenGL init.
> 17:40:10     INFO -  * Error during ANGLE OpenGL init.
> 17:40:10     INFO -  * Error during ANGLE OpenGL init.
> 17:40:10     INFO -  * Error during ANGLE OpenGL init.
> 17:40:10     INFO -  * Exhausted GL driver caps.
> 
> Please see if you can reproduce this on local Windows.

This is expected behavior with disabled HWA, see bug 1340858. What can we do here, not check on macOS and Windows or enable HWA for this tests?

> Then, both Windows and OS X show "JavaScript error:
> chrome://messenger/content/about-support/aboutSupport.js, line 665:
> TypeError: data.syscallLog is undefined". It seems that block wasn't made
> conditional on being on Linux (as the table in xthml file is). Maybe a bug
> in toolkit (the table is new).

I's say this is a toolkit error because syslog is Linux only. In aboutSupport.xhtml it's IFDEFed but in aboutSupport.js they do nothing special. Can the test be done to not consider it on macOS and Windows?

Comment 48

[Richard Marti (:Paenglab)]

(In reply to Richard Marti (:Paenglab) from comment #47)
> 
> I's say this is a toolkit error because syslog is Linux only. In
> aboutSupport.xhtml it's IFDEFed but in aboutSupport.js they do nothing
> special. Can the test be done to not consider it on macOS and Windows?

I could also add a | if (AppConstants.platform == "linux") {} |. We have already TB specific additions.

Comment 49

[:aceman]

Yes, please file that bug in toolkit and also add that condition into our version of the file. Also the error does not make the test fail, but it is ugly and unnecessary.

(In reply to Richard Marti (:Paenglab) from comment #47)
> This is expected behavior with disabled HWA, see bug 1340858. What can we do
> here, not check on macOS and Windows or enable HWA for this tests?

OK, but what is the default of HWA on release builds? I thought it is enabled. It is specifically disabled on try servers? Aren't we supposed to test the default configuration?

Comment 50

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #47)
> This is expected behavior with disabled HWA, see bug 1340858.
Indeed, FF reports the same and it is expected behaviour. We wanted to align, so we aligned and get the good the good and the bad. A FF Nightly shows exactly the same.

(In reply to :aceman from comment #49)
> OK, but what is the default of HWA on release builds? I thought it is
> enabled.
After much discussion it was disabled in TB 38 in bug 1131879.

IMHO the graphics section is fine as-is, assuming that WegGL 1 information is properly displayed, which I haven't tested, but I take Richard's word for it.

I'm happy to have other the Linux issue addressed.

Comment 51

[Richard Marti (:Paenglab)]

The confvars.sh part of my patch is no more needed, bug 1341459 removed this variable.

Comment 52

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

Removed the no more needed confvars.sh change and added AppConstants.platform == "linux" around the syscalls part in aboutSupport.js.

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: 1339811-test-failure.txt

The test fails locally as per the attached log.

Comment 54

[Jorg K (CEST = GMT+2)]

For the recode: As discussed on IRC, the test fails because it detects the string "null" on the page in the WebGL 2 information which contains |EGL_EXTENSIONS(nullptr)|. So removing "null" from the error strings in 
  var ABOUT_SUPPORT_ERROR_STRINGS = ["undefined", "null"];
makes the test pass. Perhaps we should have a list of exceptions or accept "nullptr" but not "null".

Comment 55

[Richard Marti (:Paenglab)]

Bug 1341957 makes some changes. I'll add this changes this evening. It has string changes, not checked deeply if they could break the actual implementation.

Comment 56

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

Included bug 1341957.

Comment 57

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

Since we opened this bug FX is very active on about:support. Added bug 1336920 and bug 1341029 (small change https://hg.mozilla.org/mozilla-central/diff/6055065a2ed5/toolkit/content/aboutSupport.js).

Comment 58

[:aceman]

Attachment: test v3

Looks like I finally gt the test to obey:)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bc3043e9ab0418c4d668dc910ca8a9ba556aed9a

Comment 59

[:aceman]

Comment on attachment 8841179 [details] [diff] [review]
aboutSupport.patch

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

Great for showing up-to-date info in about:support and that you could remove so many old files.
Works for me on Linux. And it seems you guys tested it on Windows and OS X to get the GPU section into shape. Thanks.

Comment 60

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841179 [details] [diff] [review]
aboutSupport.patch

Let me give the last version a quick test. Building now.

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841183 [details] [diff] [review]
test v3

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

I will give it a final test once my build is done.

::: mail/test/mozmill/content-tabs/test-about-support.js
@@ +253,5 @@
>      let text = data.value.QueryInterface(Ci.nsISupportsString).data;
> +    let contentBody;
> +    if (flavor == "text/html") {
> +      let doc = mc.window.document.implementation
> +                  .createHTMLDocument("TempDocumentWithCopiedData");

You're making this very complicated. Do you need to create a document for that? Can't you just search the stings in the HTML text that you have? You're looking for "undefined" and "null", so I don't think you run the risk of finding this text in tags. Anyway, I'm not the expert here, but on other occasions we use:
doc = (new DOMParser()).parseFromString(s, 'text/html');

Comment 62

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841183 [details] [diff] [review]
test v3

Yep, pressed all the buttons ;-)

Comment 63

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841179 [details] [diff] [review]
aboutSupport.patch

Sorry, wrong patch.

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: 1339811-test.patch (v3b).

Any objection to this?

Comment 65

[:aceman]

Should be fine if it creates a DOM document too.
Just use double quotes in 'text/html' please. Althought https://dxr.mozilla.org/comm-central/rev/be661bae6cb9a53935c5b87744bf68879d9ebcc5/mozilla/dom/base/nsIDOMParser.idl#57 does not list text/html as a legal value. Can you check that? You may want "application/xhtml+xml".
Hpefully the new aboutSupport.xhtml and all the stuff dumped into it is valid xhtml.

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: 1339811-test.patch (v3c).

Sorry about the single quotes (pasted it from the wrong original). I'm sure "text/html" works:
https://dxr.mozilla.org/comm-central/search?q=regexp%3AparseFromString.*html&redirect=false

Comment 67

[:aceman]

OK, html seems special-cased at https://dxr.mozilla.org/comm-central/rev/be661bae6cb9a53935c5b87744bf68879d9ebcc5/mozilla/dom/base/DOMParser.cpp#90 and not forwarded to ParseFromStream (which does reject text/html).

Comment 68

[Jorg K (CEST = GMT+2)]

Are we ready to land here? Who'd like to do so?

Comment 69

[:aceman]

https://hg.mozilla.org/comm-central/rev/e474c682bc73ff980a0716d4239aaa4e959b1657
https://hg.mozilla.org/comm-central/rev/996f420f761c3d6b022e3c0834ca1f6ba6ab3efd

Thanks for the teamwork ;)

Comment 70

[:aceman]

Attachment: fix for xpcshell test

I missed the xpcshell test. It needs including the account.js code in a different way now that aboutSupport.js is no longer a module.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=103d78dbb41aeab7fcd0665f423e53f40317aaa5

Comment 71

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841275 [details] [diff] [review]
fix for xpcshell test

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

I think I'm really the wrong reviewer for this, if you want to live with rs=jorgk, it's OK, otherwise, try Magnus.

::: mail/components/about-support/content/accounts.js
@@ +223,5 @@
>  
>  /**
>   * A list of account details.
>   */
> +var gAccountDetails = AboutSupport.getAccountDetails();

Care to explain this change?

::: mail/components/test/unit/test_about_support.js
@@ +136,5 @@
>   * we don't have any information about what sort of file system we're running
>   * on.
>   */
>  function test_get_file_system_type() {
> +  let fsType = AboutSupportPlatform.getFileSystemType(do_get_cwd());

... and this one.

Comment 72

[:aceman]

Comment on attachment 8841275 [details] [diff] [review]
fix for xpcshell test

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

::: mail/components/about-support/content/accounts.js
@@ +223,5 @@
>  
>  /**
>   * A list of account details.
>   */
> +var gAccountDetails = AboutSupport.getAccountDetails();

The 'window' didn't exist in xpcshell so I had to bind gAccountDetails to something else. It seems it works as a normal global variable fine.

::: mail/components/test/unit/test_about_support.js
@@ +136,5 @@
>   * we don't have any information about what sort of file system we're running
>   * on.
>   */
>  function test_get_file_system_type() {
> +  let fsType = AboutSupportPlatform.getFileSystemType(do_get_cwd());

getFileSystemType is a method of AboutSupportPlatform, not AboutSupport now. We could make it part of AboutSupport as it was before, but I see no need.

Comment 73

[:aceman]

Thanks.

Comment 74

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e474c682bc73ff980a0716d4239aaa4e959b1657 (comment #69)
https://hg.mozilla.org/comm-central/rev/996f420f761c3d6b022e3c0834ca1f6ba6ab3efd (comment #69)
https://hg.mozilla.org/comm-central/rev/3b0b98c11b7bb7ccb0d52599cd8dd04518c8b872