Help | troubleshooting yields chrome://messenger/content/about-support/gfx.js:108:33 NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]

RESOLVED FIXED in Thunderbird 54.0

Status

Thunderbird
General
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+2))

Tracking

({regression})

54 Branch
Thunderbird 54.0
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

With current nightly 2017-02-11 I'm seeing something similar to Bug #1281785

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]  gfx.js:108
	pushFeatureInfoRow chrome://messenger/content/about-support/gfx.js:108:33
	populateGraphicsSection chrome://messenger/content/about-support/gfx.js:201:5
	window.onload chrome://messenger/content/about-support/init.js:74:3
(Assignee)

Comment 1

4 months ago
Alice, could you please locate the regression here.
Flags: needinfo?(alice0775)
Keywords: regression
Regression from bug 1335296.
Flags: needinfo?(alice0775)
(Assignee)

Comment 3

4 months ago
Created attachment 8837182 [details] [diff] [review]
1339436-string-change.patch

Easy review: One character change ported from
https://hg.mozilla.org/mozilla-central/rev/9462a96e05f4#l22.36
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8837182 - Flags: review?(richard.marti)
Comment on attachment 8837182 [details] [diff] [review]
1339436-string-change.patch

Works again. Thank you.
Attachment #8837182 - Flags: review?(richard.marti) → review+
(Assignee)

Comment 5

4 months ago
https://hg.mozilla.org/comm-central/rev/a7db0899f6a4eb581aca3e63668823a416111503
Oops, commit message should have read "adapt" not "adopt".
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
(Assignee)

Comment 6

4 months ago
Created attachment 8837531 [details] [diff] [review]
WIP: 1339436-trouble-shooting.patch

Since the troubleshooting information breaks all the time (as mentioned, it broke in bug 1281785 but also bug 1302651 and bug 1302701) we need a very simple test for it that opens "about:support" and then checks the existence of these items, IDs from:

https://dxr.mozilla.org/comm-central/source/mail/components/about-support/content/aboutSupport.xhtml

Application Basics     id="application-box"
Mail and News Accounts id="accounts-table"
Crash Reports          id="crashes-table"
Extensions             id="extensions-table"
Important Modified Preferences id="prefs-tbody"
Graphics               id="graphics-tbody", id="graphics-info-properties",
                       id="graphics-failures-tbody"
JavaScript             id="javascript-incremental-gc"
Accessibility          id="a11y-activated", id="a11y-force-disabled"
Library Versions       id="libversions-tbody"

Typically when something breaks, sections go missing.

Aceman, I'm having a bad time creating a test so can you please help me. First I tried to use the application menu that I have used in other tests. Somehow it can find "View" and "Tools" but it fails to find "Help" which is the last item on the menu. Message:
https://dxr.mozilla.org/comm-central/rev/424cdd3c5239dd57a3110b6468bb79ca9d005eaa/mail/test/mozmill/shared-modules/test-window-helpers.js#1008
Did not find matching menu item for action index 0: { "label": "Help" }
(I wrote this down from memory, so the real message might be slightly different)

Next I tried to call the function directly. The attached test actually passes, so the number of tabs increases from one to two. However, I can't see the support tab. Also, for example, mc.window.document.getElementById("extensions-table"); returns null.

Help!
Flags: needinfo?(acelists)
I think it would be better to use the toolkit modules for the not-mail specific parts. Then we have the newest changes in our about:support page and such changes would less break TB (hopefully).

I tried some time ago such a change but failed to adapt the mail part.
(Assignee)

Comment 8

4 months ago
Yes, I imagined another bug "Align troubleshooting information (about:support) with M-C". But having a test wouldn't hurt anyway. Would you like to file such a bug and I can help with the mail part.

Comment 9

4 months ago
Created attachment 8837754 [details] [diff] [review]
WIP: troubleshooting test v2

I found the problems in the test patch.
Clicking on splitmenu elements was unsupported (they were intentionally skipped). I had to add it into click_menus_in_sequence . Also, clicking the Help item itself would actually open a browser with Help page. We want to click the small submenu arrow. That is now done.

Then once the troubleshooting tab was being opened, it was needed to wait a bit.

I removed the skipping of splitmenu elements so that may affect some tests that rely on it. Let's see: https://hg.mozilla.org/try-comm-central/rev/faadded7bf73e55cbcd295b6cde91e394547b6a3 .
Attachment #8837531 - Attachment is obsolete: true
Flags: needinfo?(acelists)

Comment 10

4 months ago
Do we care that this simple test does not catch the bug here? The missing string caused an exception, but the section/table headers are still there, just unpopulated. It could probably catch a missing entity bug. We would need to check if the Library versions section is populated to see if all the code ran to the end.
(Assignee)

Comment 11

4 months ago
Aceman, many thanks. Would you like to take the bug from here?

My plan was to iterate over all the IDs I listed in comment #6. We could check that these elements, if found, have children. That would catch a malfunction.

I just don't want to be in a situation where a breakage goes unnoticed for more than 10 days. The breaking bug was landed on Feb 3rd and this bug was filed on Feb 14th, so no one noticed for 11 days!

So if you'll take if from here, I'm more than glad to leave it to you ;-) I'm happy to review it ;-)

Comment 12

4 months ago
Created attachment 8837772 [details] [diff] [review]
WIP: troubleshooting test v3

Would this work for you? (not done yet due to the pending try run.)
Attachment #8837754 - Attachment is obsolete: true
Attachment #8837772 - Flags: feedback?(jorgk)
(Assignee)

Comment 13

4 months ago
Comment on attachment 8837772 [details] [diff] [review]
WIP: troubleshooting test v3

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

This looks good. You could try two things:
Undo the first patch and see what fails.
Or undo this: https://hg.mozilla.org/comm-central/rev/61cdf6e8fb63

::: mail/test/mozmill/tabmail/test-troubleshooting-info.js
@@ +42,5 @@
> +  // Check if the Library versions table exists on the page.
> +  // An exception or missing string from m-c could cause the section to be missing.
> +  mc.waitFor(function() {
> +    return !!infoPage.getElementById("libversions-tbody"); },
> +    "Extensions table in about:troubleshooting not found.", 20000, 500);

This looks funny. Can you indent this differently.

@@ +46,5 @@
> +    "Extensions table in about:troubleshooting not found.", 20000, 500);
> +
> +  // Check if all tables are populated with at least one row in the tbody element.
> +  let tables = infoPage.querySelectorAll("tbody");
> +  let emptyTables = [ "graphics-failures-tbody" ]; // some tables may be empty

I hope this one is empty ;-)
Although, there were times when graphic failures occurred and then our processing didn't work and the whole page was incomplete:
https://hg.mozilla.org/comm-central/rev/4e6a8a13868c
Attachment #8837772 - Flags: feedback?(jorgk) → feedback+

Comment 14

4 months ago
(In reply to Jorg K (GMT+1) from comment #13)
> Review of attachment 8837772 [details] [diff] [review]:
> -----------------------------------------------------------------
> This looks good. You could try two things:
> Undo the first patch and see what fails.

Yes, I undid the patch in this bug and the test detected it because one of the gfx sections was empty.

> This looks funny. Can you indent this differently.
Will try.

> @@ +46,5 @@
> > +    "Extensions table in about:troubleshooting not found.", 20000, 500);
> > +
> > +  // Check if all tables are populated with at least one row in the tbody element.
> > +  let tables = infoPage.querySelectorAll("tbody");
> > +  let emptyTables = [ "graphics-failures-tbody" ]; // some tables may be empty
> 
> I hope this one is empty ;-)
It is for me (which is strange on linux :)).

> Although, there were times when graphic failures occurred and then our
> processing didn't work and the whole page was incomplete:
> https://hg.mozilla.org/comm-central/rev/4e6a8a13868c

So the test just skips the section, we don't know what to expect there. The try servers can have restricted GPU that can produce many "errors" in that section. But that is normal operation.

Comment 15

4 months ago
Created attachment 8837957 [details] [diff] [review]
troubleshooting test v4

This versions solves all problems for me (even the main splitmenu item can be clicked now).

This test does catch:
1. random exception in the code causing some tables to remain unpopulated (including this bug).
2. missing string entity, when the whole page as a XML error and no elements will be found.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=38ce678f4fb94405e1987ced429715174e7355dd

And now when all is done, I find that we already have a test for this in test-about-support.js :( It just doesn't catch the particular bug here, but does catch a missing entity. So let's start over :(
Attachment #8837772 - Attachment is obsolete: true
Attachment #8837957 - Flags: review-
(Assignee)

Comment 16

4 months ago
Not sure why you put r- onto your own work. Anyway, can you move the useful bits from this patch to the other test. And the changes to click_menus_in_sequence aren't useful?

Comment 17

4 months ago
(In reply to Jorg K (GMT+1) from comment #16)
> Not sure why you put r- onto your own work.
To mark it is not done :)

> Anyway, can you move the useful
> bits from this patch to the other test.
Yes.

> And the changes to
> click_menus_in_sequence aren't useful?
They are, as we do not like clicking on hidden items, I'll use it to open the tab in the existing test.

Comment 18

4 months ago
Created attachment 8837970 [details] [diff] [review]
troubleshooting test v5

Moved the table check to test-about-support.js.
Attachment #8837957 - Attachment is obsolete: true
Attachment #8837970 - Flags: review?(jorgk)
(Assignee)

Comment 19

4 months ago
Comment on attachment 8837970 [details] [diff] [review]
troubleshooting test v5

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

Nice! What's the next thing I can commission you to fix for me ;-) ?

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +947,5 @@
>       *     to open if it is in the process.
> +     * @param aActions  An array of objects where each object has attributes
> +     *     with a value defined. It picks the menu item whose DOM node matches
> +     *     all the attributes with the specified names and value. It clicks whatever
> +     *     we find. It throws if the element being asked for is not found.

Actually, I prefer the "we" over the "it" ;-)
We are the developers and the code does what we want. What is "it"? The program has its own free will. I hope not. Also, you're not consistent here:
  It clicks whatever we find.
So at least make that: ... it finds.
I can hear someone saying: Don't focus on comments!
Attachment #8837970 - Flags: review?(jorgk) → review+
(Assignee)

Comment 20

4 months ago
Hmm, looks like the try wasn't successful:
INFO -    EXCEPTION: Troubleshooting table 'crashes-tbody' is empty!
So another one to add. r+ for that ;-)

Comment 21

4 months ago
(In reply to Jorg K (GMT+1) from comment #19)
> Actually, I prefer the "we" over the "it" ;-)
> We are the developers and the code does what we want. What is "it"? The

"It" is the function :)

> program has its own free will. I hope not. Also, you're not consistent here:
>   It clicks whatever we find.
> So at least make that: ... it finds.
> I can hear someone saying: Don't focus on comments!

I do, the comment was wrongly saying we only check one attribute in the object.

(In reply to Jorg K (GMT+1) from comment #20)
> Hmm, looks like the try wasn't successful:
> INFO -    EXCEPTION: Troubleshooting table 'crashes-tbody' is empty!
> So another one to add. r+ for that ;-)

Yes, crashes can also be empty. If you wonder why it passed my local testing then it is because I have crash reporter compiled out and then the section is not even in the xhtml file.
(Assignee)

Comment 22

4 months ago
Gotta change the author in that patch ;-) (seen on try)

Comment 23

4 months ago
Created attachment 8838308 [details] [diff] [review]
troubleshooting test v6

I had to touch open_content_tab_with_click() a bit as the previous opening the menu with click_menus_in_sequence() and then a separate click on the "About troubleshooting" inside open_content_tab_with_click() wasn't working 100% right (even though it opened the tab).
Attachment #8837970 - Attachment is obsolete: true
Attachment #8838308 - Flags: review?(jorgk)
(Assignee)

Comment 24

4 months ago
Comment on attachment 8838308 [details] [diff] [review]
troubleshooting test v6

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

Great. Thanks.

::: mail/test/mozmill/content-tabs/test-about-support.js
@@ +52,5 @@
> +    mc.click(mc.eid("button-appmenu"));
> +    mc.click_menus_in_sequence(mc.e("appmenu-popup"),
> +                               [ { id: "appmenu_help" },
> +                                 { id: "appmenu_troubleshootingInfo" } ]);
> +  }

Nit: I think this needs to be indented:

let xxx = function() {
    // do stuff.
  }

@@ +53,5 @@
> +    mc.click_menus_in_sequence(mc.e("appmenu-popup"),
> +                               [ { id: "appmenu_help" },
> +                                 { id: "appmenu_troubleshootingInfo" } ]);
> +  }
> +  let tab = open_content_tab_with_click(openSupport, "about:support");

I think you can lose openSupport.

let tab = open_content_tab_with_click(function() {
    // do stuff
  },
  "about:support");

No?
Attachment #8838308 - Flags: review?(jorgk) → review+

Comment 25

4 months ago
Yes I can make it without the helper functions but it is totally ugly ;)

There were 2 failures in this test on Windows (https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5ae193e4f0dafbad5d62c79ff8b974fd06cded4c). Can you reproduce them locally?
(Assignee)

Comment 26

4 months ago
(In reply to :aceman from comment #25)
> Can you reproduce them locally?

No, sorry ;-(

INFO Passed: 22
INFO Failed: 0
INFO Skipped: 0
SUMMARY-PASS | test-about-support.js::setupModule
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_display_about_support
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_accounts_in_order
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_modified_pref_on_whitelist
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_modified_pref_not_on_whitelist
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_modified_pref_on_blacklist
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_private_data
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_copy_to_clipboard_public
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_copy_to_clipboard_private
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_send_via_email_public
SUMMARY-PASS | test-about-support.js::teardownTest
SUMMARY-PASS | test-about-support.js::test_send_via_email_private
SUMMARY-PASS | test-about-support.js::teardownModule
(Assignee)

Comment 27

4 months ago
Comment on attachment 8838308 [details] [diff] [review]
troubleshooting test v6

Let's move additional testing into bug 1339811.

If we want to keep the enhancements to click_menus_in_sequence(), we should do so in a new bug. OK, Aceman?
Attachment #8838308 - Attachment is obsolete: true

Comment 28

4 months ago
OK.
You need to log in before you can comment on or make changes to this bug.