800775 - Disable unsupported character encoding menu for non-HTML documents

Status: VERIFIED DUPLICATE of bug 234628

(Firefox :: General, defect)

Description

[Masahiro YAMADA]

Tested Version:
  Nightly 19.0a1 (2012-10-11)
  Built from http://hg.mozilla.org/mozilla-central/rev/99898ec9976a

Steps to Reproduce:
1. Create new profile
2. Open "about:home" or "about:newtab"
3. Change character encoding to UTF-16LE (View - Character Encoding).
3. Go to http://www.mozilla.org/en-US/about/
  (Bookmarks - Mozilla Firefox - About Us)

Expected Result:
 http://www.mozilla.org/en-US/about/ is shown correctly.

Actual Result:
 http://www.mozilla.org/en-US/about/ is shown as UTF-16LE encoded text.

Comment 1

[Masahiro YAMADA]

Alos reproduced on Firefox 15.0.1

Comment 2

[Alice0775 White]

I can reproduce with about:home and about:newtab, but cannot with abbout:blank.
There are about:home and about:newtab each different regression range.

#1 Regression window for about:home
Good:
http://hg.mozilla.org/mozilla-central/rev/75cc425bbaf0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre ID:20100826034234
Bad:
http://hg.mozilla.org/mozilla-central/rev/291cea9d9fca
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre ID:20100826035204
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75cc425bbaf0&tochange=291cea9d9fca

Triggered by: Bug 563723


#2 Regression window for about:newtab
Good because about:newtab is not implimented:
http://hg.mozilla.org/mozilla-central/rev/c6a3d396b0cd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120125 Firefox/12.0a1 ID:20120125233652
Bad:
http://hg.mozilla.org/mozilla-central/rev/402b394b6623
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120125 Firefox/12.0a1 ID:20120125234652
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c6a3d396b0cd&tochange=402b394b6623

Triggered by: Bug 455553

Comment 3

[Marco Bonardo [:mak]]

does this happen only on these 2 about pages, or on any other about page?

Comment 4

[Marco Bonardo [:mak]]

excluded blank, that is sort of special... about:rights or about:mozilla?

Comment 5

[Alice0775 White]

I can reproduce with about: , about:about, about:rights, about:mozilla, about:crashes , about:compartments, about:memory, about:sessionrestore
however, I cannot with about:blank, about:pligins, about:buildconfig, about:cache .

Comment 6

[Frank Yan (:fryn)]

(In reply to Alice0775 White from comment #2)
> #1 Regression window for about:home
> Triggered by: Bug 563723
> 
> 
> #2 Regression window for about:newtab
> Triggered by: Bug 455553

This implies that the problem is in our character encoding selection code.
In my recent mozilla-central build (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/19.0 Firefox/19.0), I see the following:
1. Navigate tab to http://www.google.com/
2. Change encoding via menu to UTF-16
3. Navigate tab to http://www.mozilla.org/
4. Character encoding menu displays "UTF-16" as checked, even though the page is not using that.

Something is broken in that code.

Comment 7

[Frank Yan (:fryn)]

Never mind. Ignore the previous comment. I misunderstood how the menu works. :P

Comment 8

[Frank Yan (:fryn)]

Discoveries from debugging:

1. Navigate to about:home
2. Change character encoding to UTF-16 (View - Character Encoding)
   * This runs `docShell.charset = "UTF-16"`, which calls nsDocShell::SetCharset.
   * Result:
     * docShell.charset doesn't actually change!
     * docShell.forcedCharset changes to "UTF-16", but nothing visually happens.
3. Navigate to http://www.tired.com/
   * This causes nsHTMLDocument::TryUserForcedCharset to be called.
     * The `if (csAtom) {` branch is taken, because docShell.forcedCharset is still "UTF-16" and didn't get reset for some reason.

Comment 9

[Frank Yan (:fryn)]

Okay, I figured it out.

When changing the character encoding via the via, only for HTML documents does nsHTMLDocument::TryUserForcedCharset get called, and when it doesn't get called, bad things happen, e.g. forcedCharset not getting reset. (The line `aDocShell->SetForcedCharset(nullptr);` is found inside that function.)

In the case of about:home, nsHTMLDocument::StartDocumentLoad is called, but it fails to get to the branch with nsHTMLDocument::TryUserForcedCharset, because isHTML() is falsy for some reason.

In the case of about:newtab and many of the other about: pages and also SVG documents, they aren't HTML documents and they don't generate HTML documents internally for viewing (like PNG images do, for example), so we don't use nsHTMLDocument at all.

I think we should just disable the character encoding menu for all pages that do not internally generate HTML documents. Alternately, we could simply have BrowserSetForcedCharacterSet do nothing in those cases, but that's bad UI.

Comment 10

[Frank Yan (:fryn)]

Ah, it breaks on about:home, because it's not actually HTML, since it uses the XML parser for its XHTML.
Another XHTML example that breaks: http://mashupguide.net/1.0/html/ch17s05.xhtml
An SVG example that breaks: http://svg-wow.org/audio/animated-lyrics.svg

I can take this bug.
How do I detect from the front-end whether the current docshell contains an nsHTMLDocument or not?

Comment 11

[Frank Yan (:fryn)]

Attachment: patch

We only support changing character encoding in HTML documents, so I am disabling
the menu in non-HTML documents. Until we have an IDL with isHTML(), we need to use this createElement hack that bz suggested and approved. I don't want to be creating temporary elements often, so I eschewed the observer model that XULBrowserWindow.isImage uses.

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 671033 [details] [diff] [review]
patch

>+# the menu otherwise. Until we have an IDL with isHTML(), we need to use this
>+# createElement hack, which is more expensive, so we are not using an observer.
Why the spec have removed .xmlVersion? :(

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 671033 [details] [diff] [review]
patch

While it's true that only HTMLDocuments actually check the "forced" charset, there are other consumers of the "default" character set (http://mxr.mozilla.org/mozilla-central/search?string=getDefaultCharacterset), which is also set by nsDocShell::SetCharset(). It looks to me like this can have side effects when you e.g. open a new HTMLDocument from a non-HTMLDocument after having switched the charset, though I'm not sure. Maybe we don't need to worry about those cases at all?

This code is definitely a mess, but I'm not sure this is the right fix. Someone who knows how the core code is supposed to work should probably comment - maybe Neil or smontagu?

Comment 14

[Frank Yan (:fryn)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #13)
> While it's true that only HTMLDocuments actually check the "forced" charset,
> there are other consumers of the "default" character set
> (http://mxr.mozilla.org/mozilla-central/
> search?string=getDefaultCharacterset), which is also set by
> nsDocShell::SetCharset(). It looks to me like this can have side effects
> when you e.g. open a new HTMLDocument from a non-HTMLDocument after having
> switched the charset, though I'm not sure. Maybe we don't need to worry
> about those cases at all?

I see. Good point. The other consumers seem to be in the implementation-detail territory. The MediaDocument reference affects UI, but not in a way that one might expect.

In my discussion with dolske and bz in #developers, we also considered simply resetting forcedCharset in nsDocument. This would fix the glitch, but it's still confusing if a user tries to change the character encoding of a non-HTMLDocument and nothing happens.

> This code is definitely a mess, but I'm not sure this is the right fix.
> Someone who knows how the core code is supposed to work should probably
> comment - maybe Neil or smontagu?

We can apply fixes on the back-end too, but as long as the user's choice isn't reflected in the document, the menu should still be disabled, I think. We already disable it for image documents, even though MediaDocument is one of the consumers of the "default" character set (though it only consumes it in StartDocumentLoad). A different front-end fix would do if this one is too hacky.

To which Neil are you referring? #NameCollision

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(the non-Deakin one)

Comment 16

[Justin Dolske [:Dolske]]

While this sure sounds like a bug, what's the real-world impact?

AIUI the character-encoding menu settings are not sticky, and I'd assume only rarely would users fiddle with this menu unless a page is broken (and even then only in they're in one of the locales where using this menu is relatively commonplace).

Did xhtml/svg documents on-the-web actually regress, or has it always been this way?

Comment 17

[Simon Montagu :smontagu]

This has morphed into more or less a duplicate of bug 234628, no?

Comment 18

[Frank Yan (:fryn)]

(In reply to Simon Montagu from comment #17)
> This has morphed into more or less a duplicate of bug 234628, no?

Yes, we can mark this as a duplicate of that. You're assigned to that bug. Are you actively working on it?

Comment 19

[Simon Montagu :smontagu]

No I'm not: feel free to steal it!

Comment 20

[Masahiro YAMADA]

->v.