1088710 - Make character encoding widget work in e10s

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

As far as I can tell from the work in bug 1080787, the button/menu itself is just not working properly, and that's why the tests are failing.

Comment 2

[:Gijs (he/him)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> 
> *** This bug has been marked as a duplicate of bug 1088540 ***

Err, this really isn't a dupe. It's still broken and the tests are indicative of this brokenness.

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1088710 - part 1: fix some leftover gunk from pre-australis menubutton, r?mconley

Review commit: https://reviewboard.mozilla.org/r/36303/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36303/

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley

Review commit: https://reviewboard.mozilla.org/r/36305/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36305/

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> Created attachment 8723023 [details]
> MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley
> 
> Review commit: https://reviewboard.mozilla.org/r/36305/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/36305/

The trypush shows that one of these tests (but not the other?) still fails on Linux. I just re-ran that test locally on Windows, where I wrote the patch, and it passes, even if I run the directory up to that test. I don't know why it would behave differently on Linux. There are no Windows try results yet. I'll look at it some more tomorrow (and potentially investigate on my linux vm) to see what's going on here.

In general, what this cset is fixing is that onLocationChange can fire before the document's charset has been set appropriately, and in e10s we never update it again after that. Which then breaks the test's expectation that the charset is unicode, rather than 'western' (iso-8859-1). I don't know why this fix would somehow work on my Windows machine and not on the Linux machine, unless httpd.js is sending a charset as a http header by default, and that charset changes per OS, or something?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8723022 [details]
MozReview Request: Bug 1088710 - part 1: fix some leftover gunk from pre-australis menubutton, r?mconley

https://reviewboard.mozilla.org/r/36303/#review32965

r=me with caveats - see below. Thanks for cleaning this up!

::: toolkit/modules/CharsetMenu.jsm:120
(Diff revision 1)
> -  build: function(parent, showAccessKeys=true, showDetector=true) {
> +  build: function(parent, showDetector=true) {

This is going to break other consumers of CharsetMenu.build. I can see that both SeaMonkey and Thunderbird use it. Both are passing "true" as the second argument, so I would wager that there aren't too many consumers passing "false" for the second argument... (I just checked add-ons MXR and I only see a few users there as well).

Can we detect when the second argument is being passed, and show a Deprecated warning, and just ignore it instead? And then maybe file bugs to update the SM / TB callers?

Then maybe set addon-compat on this bug, or maybe dev-doc-needed or something?

Comment 7

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> Can we detect when the second argument is being passed, and show a
> Deprecated warning, and just ignore it instead? And then maybe file bugs to
> update the SM / TB callers?

There's a third argument that becomes the second argument post-patch (an argument actually still actively used by seamonkey/thunderbird code), so I don't really see how that should work.

I could keep the argument as-is and just make it no-op and add a comment about that? Would that be better?

(In reply to :Gijs Kruitbosch from comment #5)
> The trypush shows that one of these tests (but not the other?) still fails
> on Linux. I just re-ran that test locally on Windows, where I wrote the
> patch, and it passes, even if I run the directory up to that test. I don't
> know why it would behave differently on Linux. There are no Windows try
> results yet. I'll look at it some more tomorrow (and potentially investigate
> on my linux vm) to see what's going on here.
> 
> In general, what this cset is fixing is that onLocationChange can fire
> before the document's charset has been set appropriately, and in e10s we
> never update it again after that. Which then breaks the test's expectation
> that the charset is unicode, rather than 'western' (iso-8859-1). I don't
> know why this fix would somehow work on my Windows machine and not on the
> Linux machine, unless httpd.js is sending a charset as a http header by
> default, and that charset changes per OS, or something?

The test passes on my local linux vm as well. Not sure why this would be different on infra. Any ideas? :-\

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> > Can we detect when the second argument is being passed, and show a
> > Deprecated warning, and just ignore it instead? And then maybe file bugs to
> > update the SM / TB callers?
> 
> There's a third argument that becomes the second argument post-patch (an
> argument actually still actively used by seamonkey/thunderbird code), so I
> don't really see how that should work.
> 
> I could keep the argument as-is and just make it no-op and add a comment
> about that? Would that be better?
> 

Yeah, I guess that's what I meant. :)

> (In reply to :Gijs Kruitbosch from comment #5)
> > The trypush shows that one of these tests (but not the other?) still fails
> > on Linux. I just re-ran that test locally on Windows, where I wrote the
> > patch, and it passes, even if I run the directory up to that test. I don't
> > know why it would behave differently on Linux. There are no Windows try
> > results yet. I'll look at it some more tomorrow (and potentially investigate
> > on my linux vm) to see what's going on here.
> > 
> > In general, what this cset is fixing is that onLocationChange can fire
> > before the document's charset has been set appropriately, and in e10s we
> > never update it again after that. Which then breaks the test's expectation
> > that the charset is unicode, rather than 'western' (iso-8859-1). I don't
> > know why this fix would somehow work on my Windows machine and not on the
> > Linux machine, unless httpd.js is sending a charset as a http header by
> > default, and that charset changes per OS, or something?
> 
> The test passes on my local linux vm as well. Not sure why this would be
> different on infra. Any ideas? :-\

Sounds like a race.

Here's a screenshot from the failure: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/c338c890c14c4e62812c83726606cd35c8c7276f1e594fd66b1673e826889fa6dcc11d3fe8037787160c6f65343d726f0832d1ae41d94a8938de7d24e9cdb0e1

It looks as if we only ever run the code to properly check the current items once onViewShowing has fired. Are you sure that's fired?

I'd log the hell out of this, is how I'd approach it.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8723022 [details]
MozReview Request: Bug 1088710 - part 1: fix some leftover gunk from pre-australis menubutton, r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36303/diff/1-2/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8723023 [details]
MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36305/diff/1-2/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8723023 [details]
MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley

https://reviewboard.mozilla.org/r/36305/#review33467

\o/ Great job!

Comment 12

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> Comment on attachment 8723023 [details]
> MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley
> 
> https://reviewboard.mozilla.org/r/36305/#review33467
> 
> \o/ Great job!

Couldn't have done it without the suggestion!

... however, surprisingly, this still dies on Linux debug. Opt looks to be fine now, but not debug:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2742cf050977

The screenshot still looks the same. Which is odd because the test asserts that the subview has been shown as well, and that assertion is passing. Wat. :-\

... actually, that screenshot (and the previous one as well) looks like the page hasn't finished loading. There's text on the test page, which isn't shown in the screenshot, and the spinner is still going.

Comment 13

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1088710 - part 3: wait for STATE_STOP as well as load, r?mconley

Review commit: https://reviewboard.mozilla.org/r/36925/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36925/

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8724217 [details]
MozReview Request: Bug 1088710 - part 3: wait for STATE_STOP as well as load, r?mconley

https://reviewboard.mozilla.org/r/36925/#review33533

Thanks Gijs!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:86
(Diff revision 1)
> -  openNewForegroundTab(tabbrowser, opening = "about:blank", aWaitForLoad = true) {
> +  openNewForegroundTab(tabbrowser, opening = "about:blank", aWaitForLoad = true, aWaitForStateStop = true) {

This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:207
(Diff revision 1)
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener2, Ci.nsIWebProgressListener])

Nit: linebreak between the two interfaces please, and add a trailing comma while you're at it?

Comment 15

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/36925/#review33533

> This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false.

D'oh. I meant false; I must have brainfarted and gone with true because that's what the other default was.

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> https://reviewboard.mozilla.org/r/36925/#review33533
> 
> > This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false.
> 
> D'oh. I meant false; I must have brainfarted and gone with true because
> that's what the other default was.

with default false instead:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbd28c7f512c

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > https://reviewboard.mozilla.org/r/36925/#review33533
> > 
> > > This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false.
> > 
> > D'oh. I meant false; I must have brainfarted and gone with true because
> > that's what the other default was.
> 
> with default false instead:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbd28c7f512c

Annnd the non-e10s web progress is much pickier and needs something that's weak-reference-able, and a filter value:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5609b37687

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/23ee4ba3d927
https://hg.mozilla.org/integration/fx-team/rev/decc59e7a6d3
https://hg.mozilla.org/integration/fx-team/rev/7ac5c16b31f4

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23ee4ba3d927
https://hg.mozilla.org/mozilla-central/rev/decc59e7a6d3
https://hg.mozilla.org/mozilla-central/rev/7ac5c16b31f4

Comment 20

[:Gijs (he/him)]

Attachment: Patch for aurora

Comment 21

[:Gijs (he/him)]

Comment on attachment 8725672 [details] [diff] [review]
Patch for aurora

Approval Request Comment
[Feature/regressing bug #]: character encoding menu + widget
[User impact if declined]: they don't work correctly in e10s mode
[Describe test coverage new/current, TreeHerder]: this includes fixing and enabling existing tests
[Risks and why]: reasonably low risk. Can't really break this more than it already is in e10s, doesn't touch non-e10s code too much
[String/UUID change made/needed]: nope

Comment 22

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Regression with e10s, tracking and marking affected for 46.

Comment 23

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8725672 [details] [diff] [review]
Patch for aurora

Has tests, and fixes an e10s regression. Yay! Please uplift.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d90312d3824a

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d90312d3824a

Comment 26

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED --> VERIFIED

Comments:
STR: 
Properly working.

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
Web page automatically loads and showing changes when encoding changes.