Closed Bug 800775 Opened 12 years ago Closed 12 years ago

Disable unsupported character encoding menu for non-HTML documents

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED DUPLICATE of bug 234628

People

(Reporter: masa141421356, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
Alos reproduced on Firefox 15.0.1
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
Blocks: 563723, 455553
Version: 16 Branch → Trunk
does this happen only on these 2 about pages, or on any other about page?
excluded blank, that is sort of special... about:rights or about:mozilla?
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 .
(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.
Never mind. Ignore the previous comment. I misunderstood how the menu works. :P
Summary: Changing character encoding on about:newtab / about:home chages character encoding of next page. → Changing character encoding on about:newtab / about:home changes character encoding of next page
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.
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.
Summary: Changing character encoding on about:newtab / about:home changes character encoding of next page → Changing character encoding on non-HTML documents and about:home changes character encoding of next page
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?
Assignee: nobody → fryn
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Summary: Changing character encoding on non-HTML documents and about:home changes character encoding of next page → Changing character encoding on a non-HTML document changes character encoding of next page
No longer blocks: 455553, 563723
Severity: minor → normal
Depends on: ParisBindings
Summary: Changing character encoding on a non-HTML document changes character encoding of next page → Disable unsupported character encoding menu for non-HTML documents
Attached patch patchSplinter Review
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.
Attachment #671033 - Flags: review?(gavin.sharp)
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 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?
Attachment #671033 - Flags: review?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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
(the non-Deakin one)
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?
This has morphed into more or less a duplicate of bug 234628, no?
(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?
No I'm not: feel free to steal it!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
->v.
Status: RESOLVED → VERIFIED
Assignee: fryn → nobody
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: