Closed Bug 1088710 Opened 5 years ago Closed 4 years ago

Make character encoding widget work in e10s

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
e10s m9+ ---
firefox46 + verified
firefox47 + fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(4 files)

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.
Flags: firefox-backlog+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1088540
(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.
tracking-e10s: --- → ?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Attachment #8723023 - Flags: review?(mconley)
(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 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?
Attachment #8723022 - Flags: review?(mconley) → review+
(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? :-\
Flags: needinfo?(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.
Flags: needinfo?(mconley)
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 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/
Attachment #8723023 - Flags: review?(mconley)
Attachment #8723023 - Flags: review?(mconley) → review+
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!
(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 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?
Attachment #8724217 - Flags: review?(mconley) → review+
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.
(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
(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
https://hg.mozilla.org/mozilla-central/rev/23ee4ba3d927
https://hg.mozilla.org/mozilla-central/rev/decc59e7a6d3
https://hg.mozilla.org/mozilla-central/rev/7ac5c16b31f4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Attached patch Patch for auroraSplinter Review
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
Attachment #8725672 - Flags: review+
Attachment #8725672 - Flags: approval-mozilla-aurora?
Regression with e10s, tracking and marking affected for 46.
Comment on attachment 8725672 [details] [diff] [review]
Patch for aurora

Has tests, and fixes an e10s regression. Yay! Please uplift.
Attachment #8725672 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.