Closed
Bug 980904
Opened 11 years ago
Closed 7 years ago
Disable the Character Encoding menu if the document is decoded as UTF-8 and has no UTF-8 errors
Categories
(Core :: DOM: HTML Parser, enhancement)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: hsivonen, Assigned: alchen)
References
Details
Attachments
(1 file, 12 obsolete files)
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. :-(
Assignee | ||
Comment 1•7 years ago
|
||
Hi Henri,
could you take a look on this patch?
Thanks.
Flags: needinfo?(hsivonen)
Comment 2•7 years ago
|
||
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.)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8920920 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → alchen
Updated•7 years ago
|
Attachment #8920920 -
Attachment is patch: true
Updated•7 years ago
|
Attachment #8920984 -
Attachment is patch: true
Comment 5•7 years ago
|
||
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/;
Reporter | ||
Comment 6•7 years ago
|
||
(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.
Flags: needinfo?(hsivonen)
Reporter | ||
Comment 7•7 years ago
|
||
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.
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?
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8920984 -
Attachment is obsolete: true
Attachment #8922269 -
Flags: review?(hsivonen)
Comment 10•7 years ago
|
||
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•7 years ago
|
||
(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.
Reporter | ||
Comment 12•7 years ago
|
||
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.
Attachment #8922269 -
Flags: review?(hsivonen) → review-
Comment 13•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
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?
Reporter | ||
Comment 16•7 years ago
|
||
(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.
Assignee | ||
Comment 17•7 years ago
|
||
Correct the mistake of previous patch.
Here is the latest version.
Thanks for your time.
Attachment #8922269 -
Attachment is obsolete: true
Attachment #8922617 -
Flags: review?(hsivonen)
Attachment #8922617 -
Flags: review?(VYV03354)
Reporter | ||
Comment 18•7 years ago
|
||
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.
Attachment #8922617 -
Flags: review?(hsivonen) → review+
Comment 19•7 years ago
|
||
> 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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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.
Attachment #8922617 -
Flags: review?(VYV03354) → review+
Assignee | ||
Comment 23•7 years ago
|
||
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)".
Attachment #8924044 -
Flags: feedback?(hsivonen)
Attachment #8924044 -
Flags: feedback?(VYV03354)
Reporter | ||
Comment 24•7 years ago
|
||
Comment on attachment 8924044 [details] [diff] [review]
Fix testcase failed
Review of attachment 8924044 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. Thanks.
Attachment #8924044 -
Flags: feedback?(hsivonen) → feedback+
Assignee | ||
Comment 25•7 years ago
|
||
(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
Flags: needinfo?(hsivonen)
Reporter | ||
Comment 26•7 years ago
|
||
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).
Flags: needinfo?(hsivonen)
Assignee | ||
Comment 27•7 years ago
|
||
We can pass all the test cases by this patch.
Please help to check it.
Thanks.
Attachment #8924044 -
Attachment is obsolete: true
Attachment #8924044 -
Flags: feedback?(VYV03354)
Attachment #8924493 -
Flags: feedback?(hsivonen)
Attachment #8924493 -
Flags: feedback?(VYV03354)
Reporter | ||
Comment 28•7 years ago
|
||
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.
Attachment #8924493 -
Flags: feedback?(hsivonen) → feedback+
Assignee | ||
Comment 29•7 years ago
|
||
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.
Attachment #8924493 -
Attachment is obsolete: true
Attachment #8924493 -
Flags: feedback?(VYV03354)
Flags: needinfo?(jaws)
Attachment #8924807 -
Flags: review?(jaws)
Assignee | ||
Comment 30•7 years ago
|
||
Comment on attachment 8924807 [details] [diff] [review]
Testcase modifications
Review of attachment 8924807 [details] [diff] [review]:
-----------------------------------------------------------------
Add Gijs for the review as well.
Attachment #8924807 -
Flags: review?(gijskruitbosch+bugs)
Comment 31•7 years ago
|
||
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?
Attachment #8924807 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Comment 32•7 years ago
|
||
(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•7 years ago
|
||
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.
Flags: needinfo?(jaws)
Attachment #8924807 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 34•7 years ago
|
||
(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).
Assignee | ||
Comment 35•7 years ago
|
||
(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•7 years ago
|
||
(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.
Assignee | ||
Comment 37•7 years ago
|
||
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?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 38•7 years ago
|
||
(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.
Flags: needinfo?(hsivonen)
Comment 39•7 years ago
|
||
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 40•7 years ago
|
||
(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.
Flags: needinfo?(hsivonen)
Assignee | ||
Comment 41•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8926795 -
Attachment is patch: true
Assignee | ||
Comment 42•7 years ago
|
||
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.
Assignee | ||
Comment 43•7 years ago
|
||
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");
}
Assignee | ||
Comment 44•7 years ago
|
||
(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?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 45•7 years ago
|
||
Update the patch based on the latest code base.
Attachment #8922617 -
Attachment is obsolete: true
Attachment #8928782 -
Flags: review+
Assignee | ||
Comment 46•7 years ago
|
||
Try server looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08e23b231023afd41af2362950538560b246d840&selectedJob=144895313
Attachment #8924807 -
Attachment is obsolete: true
Attachment #8926795 -
Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8928784 -
Flags: review?(gijskruitbosch+bugs)
Comment 47•7 years ago
|
||
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.
Attachment #8928784 -
Flags: review?(gijskruitbosch+bugs) → review-
Assignee | ||
Comment 48•7 years ago
|
||
(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
Assignee | ||
Comment 49•7 years ago
|
||
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
Flags: needinfo?(gijskruitbosch+bugs)
Comment 50•7 years ago
|
||
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
Assignee | ||
Comment 51•7 years ago
|
||
Comment on attachment 8929792 [details] [diff] [review]
1116-bug-2.patch
Hi Masatoshi Kimura
Thanks for your help. :)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8929792 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8929792 -
Flags: review?(gijskruitbosch+bugs) → feedback?(gijskruitbosch+bugs)
Comment 52•7 years ago
|
||
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.
Attachment #8929792 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment 53•7 years ago
|
||
Yeah, BrowserTestUtils.browserStopped worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d04947d6f11b6ad8662faa630410c8590fd65316
Assignee | ||
Comment 54•7 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #53)
> Yeah, BrowserTestUtils.browserStopped worked:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d04947d6f11b6ad8662faa630410c8590fd65316
Attachment #8928784 -
Attachment is obsolete: true
Attachment #8929792 -
Attachment is obsolete: true
Attachment #8930780 -
Flags: review?(gijskruitbosch+bugs)
Comment 55•7 years ago
|
||
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.
Attachment #8930780 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 56•7 years ago
|
||
Try looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df88a12a05e5c82ddfc097f2eb91a9c45f1811fd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cca4b499b96e6ad47d30f170eaffe35c319b73
Attachment #8928782 -
Attachment is obsolete: true
Attachment #8930780 -
Attachment is obsolete: true
Attachment #8931261 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 57•7 years ago
|
||
Pushed by nbeleuzu@mozilla.com:
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
Keywords: checkin-needed
Comment 58•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 59•6 years ago
|
||
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.
Reporter | ||
Comment 60•6 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•