980904 - Disable the Character Encoding menu if the document is decoded as UTF-8 and has no UTF-8 errors

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Henri Sivonen (:hsivonen)]

Probably not worth spending a lot of time on, but this could reduce noise in CHARSET_OVERRIDE_USED telemetry:

Since valid UTF-8 data is very likely not to be meant to be in another encoding, it's useless to have the Character Encoding menu enabled when
 a) the document was decoded as UTF-8
AND
 b) the decoder encountered no errors.

Unfortunately, the errors are no longer exposed to the calling parser code. :-(

Comment 1

[Alphan Chen [:alchen]]

Attachment: Disable the Character Encoding menu when document is decoded as UTF-8 without errors

Hi Henri,
could you take a look on this patch?
Thanks.

Comment 2

[Anne (:annevk)]

Not sure if worth doing as part of this bug, but I think we should consider a UTF-8 BOM authoritative too, but maybe we already have that in place. (Pretty sure we do for UTF-16.)

Comment 3

[Alphan Chen [:alchen]]

Attachment: Disable the Character Encoding menu when document is decoded as UTF-8 without errors

Comment 4

[Alphan Chen [:alchen]]

(In reply to Anne (:annevk) from comment #2)
> Not sure if worth doing as part of this bug, but I think we should consider
> a UTF-8 BOM authoritative too, but maybe we already have that in place.
> (Pretty sure we do for UTF-16.)

I think these(both UTF-8 and 16) are covered by "nsHtml5StreamParser::SetupDecodingFromBom" and "nsHtml5StreamParser::SniffStreamBytes".
Need Henri's confirmation.

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 8920984 [details] [diff] [review]
Disable the Character Encoding menu when document is decoded as UTF-8 without errors

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

::: dom/html/nsHTMLDocument.cpp
@@ +842,5 @@
>  void
>  nsHTMLDocument::EndLoad()
>  {
> +  nsHtml5TreeOpExecutor* executor = nullptr;
> +  executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());

Are you sure non-HTML5 parsers will never be used here?

::: parser/html/nsHtml5StreamParser.cpp
@@ +852,1 @@
>      Unused << hadErrors;

nit: Please remove this line because now `hadErrors` is used :)

@@ +1103,1 @@
>      Unused << hadErrors;

Ditto.

::: parser/html/nsHtml5TreeOpExecutor.h
@@ +199,5 @@
>      void ComplainAboutBogusProtocolCharset(nsIDocument* aDoc);
>  
> +    void ReportDecodeError()
> +    {
> +      mDecodeError = 1;

Nit: s/1/true/;

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> >  nsHTMLDocument::EndLoad()
> >  {
> > +  nsHtml5TreeOpExecutor* executor = nullptr;
> > +  executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());
> 
> Are you sure non-HTML5 parsers will never be used here?

In addition to this problem, there is another major concern:

The decode errors are reported from the parser thread to the main thread without synchronization.

I suggest fixing this like this:

 1) Have a boolean mHasHadErrors (initialized to false) on nsHtml5StreamParser, and do mHasHadErrors |= hadErrors after each call to mUnicodeDecoder->DecodeToUTF16. 

 2) Immediately before the line "if (mCharsetSource < kCharsetFromMetaTag) {" (https://searchfox.org/mozilla-central/source/parser/html/nsHtml5StreamParser.cpp#1457) when processing EOF, add a check "if (mEncoding == UTF_8_ENCODING && !mHasHadErrors)" and send a newly-minted tree op for disabling the character encoding menu if the condition holds. (See the MaybeComplainAboutCharset mechanism immediately below for an example of how to send a tree op to the main thread.)

(In reply to Anne (:annevk) from comment #2)
> Not sure if worth doing as part of this bug, but I think we should consider
> a UTF-8 BOM authoritative too, but maybe we already have that in place.
> (Pretty sure we do for UTF-16.)

A BOM already disables the character encoding menu.

Comment 7

[Henri Sivonen (:hsivonen)]

By newly-minted, I mean adding a new item, let's call it eTreeOpDisableEncodingMenu to enum eHtml5TreeOperation and making the nsHtml5TreeOperation::Perform call a new DisableEncodingMenu() method in nsIDocument when encountering it.

Comment 8

[imer]

Is it necessary to disable encoding selection menu, even if encoding was correctly recognized as UTF-8? User will see characters which was expected, and will not even go for encoding selection menu anyway normally.

But there is an use case, when user might want the browser to display page in incorrect encoding:
Imagine you have seen mojibake in some other program and want to figure out what encoding that other program have expected in place of UTF-8.
In order to solve that you might want to load same text in browser then switch encoding until you find one which produces same kind of mojibake as the program.

Of course there are easier ways to do that, but since encoding menu doesn't hurt and have some, even if unlikely use, why not to keep it always enabled?

Comment 9

[Alphan Chen [:alchen]]

Attachment: Disable the Character Encoding menu when document is decoded as UTF-8 without errors.r?hsivonen

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 8922269 [details] [diff] [review]
Disable the Character Encoding menu when document is decoded as UTF-8 without errors.r?hsivonen

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

::: dom/html/nsHTMLDocument.cpp
@@ +3724,5 @@
>  nsHTMLDocument::WillIgnoreCharsetOverride()
>  {
> +  if (mParser) {
> +    nsHtml5TreeOpExecutor* executor = nullptr;
> +    executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());

This patch does not fix the non-HTML5 parser issue. Nor the async issue.

::: parser/html/nsHtml5TreeOpExecutor.h
@@ +197,5 @@
>                                     uint32_t aLineNumber);
>  
>      void ComplainAboutBogusProtocolCharset(nsIDocument* aDoc);
>  
> +    void DisableEncodingMenu()

Please read Henri's comment carefully. You should add DisableEncodingMenu() to nsIDocument, not to nsHtml5TreeOpExecutor.

See the `eTreeOpAddViewSourceHref` case about how to get an nsIDocument pointer.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to imer from comment #8)
> Is it necessary to disable encoding selection menu, even if encoding was
> correctly recognized as UTF-8? User will see characters which was expected,
> and will not even go for encoding selection menu anyway normally.
> 
> But there is an use case, when user might want the browser to display page
> in incorrect encoding:
> Imagine you have seen mojibake in some other program and want to figure out
> what encoding that other program have expected in place of UTF-8.
> In order to solve that you might want to load same text in browser then
> switch encoding until you find one which produces same kind of mojibake as
> the program.
> 
> Of course there are easier ways to do that, but since encoding menu doesn't
> hurt and have some, even if unlikely use, why not to keep it always enabled?

Encoding menu is a well-known self-XSS attack vector (especially ISO-2022-JP). I'd like to disable it as much as possible.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 8922269 [details] [diff] [review]
Disable the Character Encoding menu when document is decoded as UTF-8 without errors.r?hsivonen

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

r-, because the mDisableEncodingMenu boolean should go on the document instead.

::: dom/html/nsHTMLDocument.cpp
@@ +3724,5 @@
>  nsHTMLDocument::WillIgnoreCharsetOverride()
>  {
> +  if (mParser) {
> +    nsHtml5TreeOpExecutor* executor = nullptr;
> +    executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());

Please put the bit to signal that the encoding menu is disabled on the document itself instead of putting it on nsHtml5TreeOpExecutor. That way, the information stays around after the parse and there's no need to static_cast in a way that's incorrect in the case of XHTML documents.

::: parser/html/nsHtml5TreeOpExecutor.h
@@ +91,5 @@
>       * to character encoding declarations.
>       */
>      bool                          mAlreadyComplainedAboutCharset;
>  
> +    bool                          mDisableEncodingMenu;

As noted, this bool should be on the document instead.

Comment 13

[imer]

Can the problem be solved in some other way, rather than disabling encoding menu completely?

If Firefox refused to execute any script from page which contains ASCII control codes, unless encoding is set to one where they are valid, would it solve the problem?

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to imer from comment #13)
> Can the problem be solved in some other way, rather than disabling encoding
> menu completely?
> 
> If Firefox refused to execute any script from page which contains ASCII
> control codes, unless encoding is set to one where they are valid, would it
> solve the problem?

How does? Isn't encoding menu used when Firefox could not determine if the encoding is valid?

I don't understand why you insist such a super-rare use case. Web browser is not an encoding finder.

Comment 15

[imer]

There is another use-case, when page in legacy encoding coincide with UTF-16 BOM, which is not likely but not impossible. FF and FE are valid characters in many legacy encodings.

> Isn't encoding menu used when Firefox could not determine if the encoding is valid?

Usually. But it's also not absolutely guaranteed that Firefox is correct, even if it has determined what encoding was.

I understand that some encoding coincide with ASCII values for HTML special characters, which can cause some user generated content be interpreted as HTML tags. It's the problem, right?

Now a question is, why sites would want to serve pages in ASCII-incompatible encodings? Are there web-sites which actually do it (not just as proof of concept)?

Are there valid web pages which are in 8-bit encodings, but have embedded ASCII control characters such as ESC or SI/SO?

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to imer from comment #15)
> There is another use-case, when page in legacy encoding coincide with UTF-16
> BOM, which is not likely but not impossible. FF and FE are valid characters
> in many legacy encodings.

Allowing UTF-16 to be overridden is an XSS hazard if the site serves user-supplied content. Try https://hsivonen.com/test/moz/never-show-user-supplied-content-as-utf-16.htm in Firefox earlier than 24.

I understand that you have a legitimate grievance regarding UTF-8 text/plain at file: URLs, but it's not really useful to try to argue that the character encoding menu should be used more.

Comment 17

[Alphan Chen [:alchen]]

Attachment: Disable the Character Encoding menu when document is decoded as UTF-8 without errors.

Correct the mistake of previous patch.
Here is the latest version.
Thanks for your time.

Comment 18

[Henri Sivonen (:hsivonen)]

Comment on attachment 8922617 [details] [diff] [review]
Disable the Character Encoding menu when document is decoded as UTF-8 without errors.

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

Looks good. Thank you.

Comment 19

[imer]

> Allowing UTF-16 to be overridden is an XSS hazard if the site serves user-supplied content.

Yes, I understand that. It has been "solved" by not allowing user to override encoding, if it's initially recognized as UTF-16. 

But limiting user choice is not really a solution. Proper solution is to always keep User in charge, but make sure user's actions do not cause consequences which user did not intend.

Such as, check whenever page contains sequences, which are not legitimate in plain text in currently selected encoding, for example ASCII control characters other than \r, \n, \t and perhaps \f and \v, if encoding is ASCII-clean (OEM, Windows, ISO 8859 or UTF-8). If they are detected, disable interpretation of scripts.

Legitimate web-pages would not contain control sequences, so there would be no harm to them.

Going down the path of disabling Encoding menu is a slippery slope. While currently it is only disabled when it's unlikely to be of use anyway, it could eventually get increasingly disabled in circumstances where use is more and more legitimate.

Are there any security (XSS) problems with solution I have proposed above?

Comment 20

[Anne (:annevk)]

I don't think you've really sold anyone on the upsides. We want to get to a point where the encoding selection menu is no longer needed. Perhaps there's a place for something like it in the developer tools.

Comment 21

[imer]

So, are there any disadvantages in proposed solution?

Changes like that do not make feature less needed. If you remove or disable it, it doesn't mean it is needed less. It just means it becomes less available, when it's needed.

Moving encoding selection to developer tools will not improve anything, just make access to the function slower.
Text encoding Icon is already not present by default in menu by default and have to be added there manually, so you can't even argue that it will reduce interface cluttering.


>Unix was not designed to stop you from doing stupid things, because that would also stop you from doing clever things.
>    Doug Gwyn, in Introducing Regular Expressions (2012) by Michael Fitzgerald

Comment 22

[Masatoshi Kimura [:emk]]

Comment on attachment 8922617 [details] [diff] [review]
Disable the Character Encoding menu when document is decoded as UTF-8 without errors.

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

::: dom/base/nsDocument.cpp
@@ +1561,5 @@
>      mIsContentDocument(false),
>      mMightHaveStaleServoData(false),
>      mDidCallBeginLoad(false),
>      mBufferingCSPViolations(false),
> +    mDisableEncodingMenu(false),

nit: mEncodingMenuDisabled

::: parser/html/nsHtml5StreamParser.h
@@ +594,5 @@
>       * timer has already fired previously in this parse.
>       */
>      static int32_t                sTimerSubsequentDelay;
> +
> +    bool                          mHasHadErrors;

I would move this definition to just below mInitialEncodingWasFromParentFrame to save the structure size.

Comment 23

[Alphan Chen [:alchen]]

Attachment: Fix testcase failed

The following tests depend on the character encoding menu.
toolkit/content/tests/browser/browser_charsetMenu_swapBrowsers.js
browser/components/customizableui/test/browser_967000_button_charEncoding.js
browser/components/customizableui/test/browser_987640_charEncoding.js

I tried to modify the tests.
Here is the patch.
Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ca49baf91e9f06f92eaac3380eee59c8735df6

However, there is uncertain behavior.
At the end of "browser_987640_charEncoding.js", it will try to reset the encoding to the original one. After modification, the encoding begins with iso-8859-1. Then it will be forced to utf-8. In the end, the test will try to reset it to iso-8859-1.

If we didn't meet encoding error in step 2(forced to utf-8), we would disable to encoding menu. So encoding should stay in utf-8. From the result of try server, the test failed on "Linux x64 opt(tc-M-e10s tier2)" and "Windows 7 debug(tc-M bc6)".

Comment 24

[Henri Sivonen (:hsivonen)]

Comment on attachment 8924044 [details] [diff] [review]
Fix testcase failed

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

Looks good to me. Thanks.

Comment 25

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #23)
> However, there is uncertain behavior.
> At the end of "browser_987640_charEncoding.js", it will try to reset the
> encoding to the original one. After modification, the encoding begins with
> iso-8859-1. Then it will be forced to utf-8. In the end, the test will try
> to reset it to iso-8859-1.
> 
> If we didn't meet encoding error in step 2(forced to utf-8), we would
> disable to encoding menu. So encoding should stay in utf-8. From the result
> of try server, the test failed on "Linux x64 opt(tc-M-e10s tier2)" and
> "Windows 7 debug(tc-M bc6)".

Hi Henri,
any thought about the failed case in comment 23? 
The failure happened randomly.
In another trial, it happened on "Linux x64 Stylo Disabled opt" and "Windows 7 debug(tc-M bc5)"
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1567fbf77f0030ef4a963c198d05ff827ea489ee

Comment 26

[Henri Sivonen (:hsivonen)]

I think it makes sense to let the menu get disabled even if UTF-8 was chosen manually and the menu wasn't disabled to begin with. It might be a little confusing, but it should reduce useless use of the menu. (Telemetry shows that 29% of encoding menu invocations are corrections of previous menu choice.)

Randomness might be due to a race condition between the disabling the menu and the test case accessing the menu. I don't know what exactly await tabLoadPromise; waits for. The test case needs to wait for the load to complete instead of just waiting for the load to start (which was previously sufficient).

Comment 27

[Alphan Chen [:alchen]]

Attachment: Fix testcase failed

We can pass all the test cases by this patch.
Please help to check it.
Thanks.

Comment 28

[Henri Sivonen (:hsivonen)]

Comment on attachment 8924493 [details] [diff] [review]
Fix testcase failed

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

Looks OK enough for me. Gijs might be able to assist in making the test deterministic.

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +51,5 @@
>    await document.getElementById("nav-bar").overflowable.show();
>    charEncodingButton.click();
>    checkedButtons = characterEncodingView.querySelectorAll("toolbarbutton[checked='true']");
> +
> +  // The encoding menu would be disabled if the charset is utf-8 without decoding error.

Please add a comment here about suspected race condition.

Comment 29

[Alphan Chen [:alchen]]

Attachment: Testcase modifications

Hi Jared,
Since Gijs is PTO now,  could you be the reviewer for this modification?

With Attachment 8922617 [details] [diff], the encoding menu will be disabled if the charset is utf-8 without decoding error.

In these three test cases, they all set the charset as utf-8 and would like to access encoding menu after load. So I make some changes and let the test cases could have a similar behavior as they did before.

Comment 30

[Alphan Chen [:alchen]]

Comment on attachment 8924807 [details] [diff] [review]
Testcase modifications

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

Add Gijs for the review as well.

Comment 31

[:Gijs (he/him)]

Comment on attachment 8924807 [details] [diff] [review]
Testcase modifications

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

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +54,5 @@
> +
> +  // The encoding menu will be disabled if the charset is utf-8 without decoding error.
> +  // There is a suspected race condition between the disabling the menu and the
> +  // test case accessing the menu.
> +  // Add an judgement to avoid randomness failures.

Can you elaborate on what causes the race condition here?

Comment 32

[Henri Sivonen (:hsivonen)]

(In reply to :Gijs (slow, PTO recovery mode) from comment #31)
> Can you elaborate on what causes the race condition here?

The test should now wait until the document has completed loading. Previously, it was sufficient for the load to have proceeded to the point where the document appears in the docshell.

Per comment 26, it's unclear to me what exactly the test waits for before it takes a look at the menu. If it's already waiting for the document to complete loading, I don't know what's wrong.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8924807 [details] [diff] [review]
Testcase modifications

So, you're running into a bug in the test. `promiseTabLoadEvent` returns `BrowserTestUtils.browserLoaded(browser)`. The first issue here is that it should probably pass the URL to wait for. But actually, before it returns that, it calls BrowserTestUtils.loadURI(...). So what's happening in the test is we do:

1) find some encoding button node
2) create a load promise that resolves when the 'load' event fires for the content in that browser *and* trigger a load of the TEST_PAGE url
3) click the encoding button node (!)

so it makes sense, kind of, that the result races. In fact, I'm a little surprised the whole thing isn't racier than it already is.

Instead of using `promiseTabLoadEvent`, just use:

BrowserTestUtils.browserLoaded(browser, false, TEST_PAGE);

and I *think* this issue will go away.

Comment 34

[Alphan Chen [:alchen]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #33)
> Comment on attachment 8924807 [details] [diff] [review]
> Testcase modifications
> 
> Instead of using `promiseTabLoadEvent`, just use:
> 
> BrowserTestUtils.browserLoaded(browser, false, TEST_PAGE);
> 
> and I *think* this issue will go away.

Update result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a862c365a4fadce357e7a1600727440dbb94978e

After using "BrowserTestUtils.browserLoaded", we can see a failure on windows 7 debug(tc-M bc7) and several timeouts on windows platforms(tc-e10s TV).

timeout: Uncaught exception - Subview (PanelUI-characterEncodingView) did not show within 20 seconds. 

I am trying to reproduce the symptom in my local side(win 10).

Comment 35

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #34)
> I am trying to reproduce the symptom in my local side(win 10).
Cannot reproduce on win10 with or without disabling e10s.

After doing more trials on try server, we can find the failure and timeout consistently.

Failed on windows7 debug version with disabling e10s.
+    "test-windows7-32/debug-mochitest-browser-chrome-7",

Timeout on the following:
+    "test-windows10-64/debug-test-verify-e10s",
+    "test-windows10-64/opt-test-verify-e10s",
+    "test-windows7-32/debug-test-verify-e10s",
+    "test-windows7-32/opt-test-verify-e10s"
What is the difference between test-verify and others?

Comment 36

[:Gijs (he/him)]

(In reply to Alphan Chen [:alchen] from comment #35)
> (In reply to Alphan Chen [:alchen] from comment #34)
> > I am trying to reproduce the symptom in my local side(win 10).
> Cannot reproduce on win10 with or without disabling e10s.
> 
> After doing more trials on try server, we can find the failure and timeout
> consistently.
> 
> Failed on windows7 debug version with disabling e10s.
> +    "test-windows7-32/debug-mochitest-browser-chrome-7",
> 
> Timeout on the following:
> +    "test-windows10-64/debug-test-verify-e10s",
> +    "test-windows10-64/opt-test-verify-e10s",
> +    "test-windows7-32/debug-test-verify-e10s",
> +    "test-windows7-32/opt-test-verify-e10s"
> What is the difference between test-verify and others?

You can run tests in verify mode by using the --verify flag. It does a bunch of verification to avoid intermittent issues in tests, which include running the same test multiple times in succession (while keeping the browser open). It's possible we're triggering that here if we're not cleaning up after ourselves correctly in the test, or something.

Comment 37

[Alphan Chen [:alchen]]

Attachment: Use BrowserTestUtils.browserLoaded instead of promiseTabLoadEvent

Hi Gijs,
By using "BrowserTestUtils.browserLoaded", the randomness failure is gone.
However, the modification causes these timeouts.

However, we meet timeout on the following.
+    "test-windows10-64/debug-test-verify-e10s",
+    "test-windows10-64/opt-test-verify-e10s",
+    "test-windows7-32/debug-test-verify-e10s",
+    "test-windows7-32/opt-test-verify-e10s"

Any suggestion?

Comment 38

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #35)
> (In reply to Alphan Chen [:alchen] from comment #34)
> > I am trying to reproduce the symptom in my local side(win 10).
> Cannot reproduce on win10 with or without disabling e10s.
> 
> 
> Failed on windows7 debug version with disabling e10s.
> +    "test-windows7-32/debug-mochitest-browser-chrome-7",
> 

Hi Henry,
any idea about this failure, which only happened on non-e10s windows7 debug version?
The menu is not been disabled.
I'll try to set up a windows7 machine to reproduce this symptom.

Comment 39

[:Gijs (he/him)]

(In reply to Alphan Chen [:alchen] from comment #37)
> Created attachment 8926795 [details] [diff] [review]
> Use BrowserTestUtils.browserLoaded instead of promiseTabLoadEvent
> 
> Hi Gijs,
> By using "BrowserTestUtils.browserLoaded", the randomness failure is gone.
> However, the modification causes these timeouts.
> 
> However, we meet timeout on the following.
> +    "test-windows10-64/debug-test-verify-e10s",
> +    "test-windows10-64/opt-test-verify-e10s",
> +    "test-windows7-32/debug-test-verify-e10s",
> +    "test-windows7-32/opt-test-verify-e10s"
> 
> Any suggestion?

Run the tests locally with --verify and see where/why they're timing out? Without more info here it's hard to comment, and I won't have access to a win7 machine until next week.

Comment 40

[Henri Sivonen (:hsivonen)]

(In reply to Alphan Chen [:alchen] from comment #38)
> any idea about this failure, which only happened on non-e10s windows7 debug
> version?

Sorry, I have no idea why it fails. :-(

At some point, if manual testing shows that the patch works, it probably makes sense to make the test cases accept non-deterministic results, since it doesn't make sense to put a huge amount of time into polishing tests for a UI features that's almost unused.

Comment 41

[Alphan Chen [:alchen]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #39)
> (In reply to Alphan Chen [:alchen] from comment #37)
> > Created attachment 8926795 [details] [diff] [review]
> > Use BrowserTestUtils.browserLoaded instead of promiseTabLoadEvent
> > Hi Gijs,
> > By using "BrowserTestUtils.browserLoaded", the randomness failure is gone.
> > However, the modification causes these timeouts.
> 
> Run the tests locally with --verify and see where/why they're timing out?
> Without more info here it's hard to comment, and I won't have access to a
> win7 machine until next week.

Update the latest status.
First of all, I cannot reproduce the symptom locally. (with --verify)

It is not related to "BrowserTestUtils.browserLoaded".
We still can meet timeout symptom on test verification with a little modification.

In this patch, I just remove the last part. We still meet timeout symptom in test verification.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcd51cd461432457cb9d9b0fcaaffd3ee460c43b

The timeout logs are all similar. (https://pastebin.mozilla.org/9072716)
Cannot find any clue from the logs.
Full LOG : https://treeherder.mozilla.org/logviewer.html#?job_id=144528806&repo=try&lineNumber=2103

The timeout symptom should exist before and is not related to our change.

Comment 42

[Alphan Chen [:alchen]]

Comment on attachment 8926795 [details] [diff] [review]
Use BrowserTestUtils.browserLoaded instead of promiseTabLoadEvent

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

In the normal usage, if the "charEncodingButton"(encoding menu) is disabled, the user cannot change the encoding even if the encoding button is enabled.
There are different behaviors of non-e10s window7 and e10s windows7.
I have no idea why non-e10s windows7 version is different from other version.

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +45,2 @@
>    await document.getElementById("nav-bar").overflowable.show();
> +  is(initialEncoding.getAttribute("disabled"), "true", "The encoding menu should be disabled");

I also find the reason of the failure on non-e10s windows7 debug version.

On non-e10s windows7 version, the "initialEncoding" is not disabled.
However, "charEncodingButton" is disabled.

Comment 43

[Alphan Chen [:alchen]]

Comment on attachment 8924807 [details] [diff] [review]
Testcase modifications

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

Based on comment 42, I would like to modify the judgment to the following.

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +59,5 @@
> +  if (initialEncoding.getAttribute("disabled")) {
> +    is(checkedButtons[0].getAttribute("label"), "Unicode", "The encoding was reset to Unicode");
> +  } else {
> +    is(checkedButtons[0].getAttribute("label"), "Western", "The encoding was reset to Western");
> +  }

if (charEncodingButton.getAttribute("disabled")) {
    // Don't need to check encoding button if menu button is disabled.
  } else {
    // Check encoding button if menu button is not disabled.
    is(initialEncoding.getAttribute("disabled"), "true", "We should disable the button");
  }

Comment 44

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #43)
> Comment on attachment 8924807 [details] [diff] [review]
> Testcase modifications
> 
> Review of attachment 8924807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Based on comment 42, I would like to modify the judgment to the following.
> 
> 
> if (charEncodingButton.getAttribute("disabled")) {
>     // Don't need to check encoding button if menu button is disabled.
>   } else {
>     // Check encoding button if menu button is not disabled.
>     is(initialEncoding.getAttribute("disabled"), "true", "We should disable
> the button");
>   }

Any advise?

Comment 45

[Alphan Chen [:alchen]]

Attachment: Disable the Character Encoding menu when document is decoded as UTF-8 without errors. r=hsivonen,emk

Update the patch based on the latest code base.

Comment 46

[Alphan Chen [:alchen]]

Attachment: Testcase modifications

Try server looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08e23b231023afd41af2362950538560b246d840&selectedJob=144895313

Comment 47

[:Gijs (he/him)]

Comment on attachment 8928784 [details] [diff] [review]
Testcase modifications

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

I don't understand this test patch.

AFAICT, the goal of this bug is that, if the button is in the overflow menu, it gets disabled when a page is loaded as UTF-8 without errors. If the button is in the toolbar, all the entries in the panel get disabled if it's shown on a page with non-error'd UTF-8 (this avoids having to have a page load listener that updates the enabled/disabled state all the time).

For one, I think it's interesting that we do this even when the user overrides the page's encoding with UTF-8, but let's leave that for now...

The test changes to browser/components/customizableui/test/browser_987640_charEncoding.js don't really make sense:

+  // check the encoding menu again
+  if (charEncodingButton.getAttribute("disabled")) {
+    // When menu is disabled, we don't need to check encoding button.
+  } else {
+    // When menu is not disabled, we neeed to make sure encoding button is disabled.
+    is(initialEncoding.getAttribute("disabled"), "true", "We should disable the button");
+  }
+

AFAICT the explicit goal of this bug is that the parent (charEncodingButton in this case) gets disabled (because it's in the overflow panel). So we should be verifying that in all cases, charEncodingButton.getAttribute("disabled") == true. We shouldn't care about the submenu. But right now we seem to mostly care about the submenu, and ignore it when the charEncodingButton is disabled (which we're not checking for)? So I don't understand how we *ever* hit the else clause here if the first patch is working. The page has been reloaded with Unicode as the selected encoding, we've waited for that load to complete, so the browser's `mayEnableCharacterEncodingMenu` bool should be false, so the button should be disabled. If that isn't working, we're not achieving the goal of this bug. I understand test failures can be annoying, but if the test failure is pointing out that something isn't always working, I think we should investigate why that is and not wallpaper it.

If the only issue is that there's some kind of race condition where in some cases, the test code runs before the disabled attribute has been set on the button, I can try to help fix the race condition, but it'd help if we had evidence to that effect. You could get that by e.g. adding `Cu.reportError()` logging to the button code in CustomizableWidgets.jsm that normally disables the button, as well as logging `gBrowser.selectedBrowser.mayEnableCharacterEncodingMenu` from within the test after the browserLoaded() promise has resolved, to figure out what's going wrong here.

Comment 48

[Alphan Chen [:alchen]]

(In reply to :Gijs (away until Nov 20) from comment #47)
> You could get that by e.g. adding
> `Cu.reportError()` logging to the button code in CustomizableWidgets.jsm
> that normally disables the button, as well as logging
> `gBrowser.selectedBrowser.mayEnableCharacterEncodingMenu` from within the
> test after the browserLoaded() promise has resolved, to figure out what's
> going wrong here.

Thanks for your advise.
It is a timing issue.

On non-e10s mode, the menu will be disabled when receiving "popupshowing" event.
https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#853
However, on e10s mode, `gBrowser.selectedBrowser.mayEnableCharacterEncodingMenu` is still true when receiving "popupshowing" event.

> So I don't understand how we *ever* hit the else clause here if the
> first patch is working. 
Currently, e10s mode will hit the else clause.
The encoding button is disabled by updateCurrentCharset().
https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#763

Comment 49

[Alphan Chen [:alchen]]

Hi Gijs,
Where can we update the "characterencoding-button" to avoid this timing issue?

I don't know if it is a good idea to add "characterencoding-button" into elements of updateCurrentCharset().
https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#769

Comment 50

[Masatoshi Kimura [:emk]]

Attachment: 1116-bug-2.patch

In e10s parent process, mayEnableCharacterEncodingMenu will not be updated until the parent process receives "Content:StateChange" message:
https://dxr.mozilla.org/mozilla-central/rev/d4753dc14b2ab9c42123b6d60a68106df40f45cd/toolkit/modules/RemoteWebProgress.jsm#235
So BrowserTestUtils.browserLoaded is useless here.

You will have to listen nsIWebProgress.onStateChange and wait until `webProgress.isTopLevel` is true and `flags` has Ci.nsIWebProgressListener.STATE_STOP. This will work regardless of the e10s setting. See the attachment for details.

Try run (windows10-64-ccov debug failures are unrelated with this bug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e4affab3ca88ef451ba56049897393eb202e67d

Comment 51

[Alphan Chen [:alchen]]

Comment on attachment 8929792 [details] [diff] [review]
1116-bug-2.patch

Hi Masatoshi Kimura
Thanks for your help. :)

Comment 52

[:Gijs (he/him)]

Comment on attachment 8929792 [details] [diff] [review]
1116-bug-2.patch

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

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +46,2 @@
>    newEncoding.click();
> +  await stateChangePromise

Instead of all this, I think you can just use BrowserTestUtils.browserStopped() ? That already does these checks and returns a promise.

Comment 53

[Masatoshi Kimura [:emk]]

Yeah, BrowserTestUtils.browserStopped worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d04947d6f11b6ad8662faa630410c8590fd65316

Comment 54

[Alphan Chen [:alchen]]

Attachment: Testcase modifications. r?Gijs

(In reply to Masatoshi Kimura [:emk] from comment #53)
> Yeah, BrowserTestUtils.browserStopped worked:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d04947d6f11b6ad8662faa630410c8590fd65316

Comment 55

[:Gijs (he/him)]

Comment on attachment 8930780 [details] [diff] [review]
Testcase modifications. r?Gijs

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

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +50,2 @@
>    await document.getElementById("nav-bar").overflowable.show();
> +  await waitForOverflowButtonShown();

This line should go before `overflowable.show()` - it waits for the ">>" button to be painted, otherwise the overflow panel (which contains the button) either doesn't anchor correctly or won't open at all.

Comment 56

[Alphan Chen [:alchen]]

Attachment: Disable the Character Encoding menu when document is decoded as UTF-8 without errors. r=hsivonen,emk,Gijs

Try looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df88a12a05e5c82ddfc097f2eb91a9c45f1811fd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cca4b499b96e6ad47d30f170eaffe35c319b73

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8281f5415cfc
Disable the Character Encoding menu when document is decoded as UTF-8 without errors. r=hsivonen,emk,Gijs

Comment 58

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/8281f5415cfc

Comment 59

[David Spector]

Please enable the View > Text Encoding menu in Firefox and add all charsets of interest, especially UTF-8, which is NOT 16-bit Unicode. Test case: page "http://webcache.googleusercontent.com/search?q=cache:n1ZQwgYsi9wJ:www.zehnet.de/2005/02/12/unicode-utf-8-tutorial/+&cd=14&hl=en&ct=clnk&gl=us&client=firefox-b-1-ab" shows UTF-8 examples that render incorrectly, but the Encoding menu is disabled, so the page is almost useless.

Comment 60

[Henri Sivonen (:hsivonen)]

(In reply to David Spector from comment #59)
> Please enable the View > Text Encoding menu in Firefox and add all charsets
> of interest, especially UTF-8, which is NOT 16-bit Unicode. Test case: page
> "http://webcache.googleusercontent.com/search?q=cache:n1ZQwgYsi9wJ:www.
> zehnet.de/2005/02/12/unicode-utf-8-tutorial/
> +&cd=14&hl=en&ct=clnk&gl=us&client=firefox-b-1-ab" shows UTF-8 examples that
> render incorrectly, but the Encoding menu is disabled, so the page is almost
> useless.

Please don't post the same comment on multiple bugs. The follow-up is in bug 1438779.