20315 - [dogfood] Encoding setting not effective in all frames

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P3)

Description

[Katsuhiko Momoi]

** Observed with 11/29/99 Win32 build (1999112908) **
** This was contributed orginally by a Mozilla user via
   the Mozilla I18n newsgroup **

The above site is encoded in Big5 and contains 3 frames -- the top,
the middle,and the bottom frames.
The middle frame contains a pop-up menu using FORM.
The menu options all have Chinese names. These names don't show correctly
even when the Encoding menu is set to Chinese Traditional (Big5).

To reproduce this bug:

1. Have Traditional Chinsese fonts installed on your Windows system.
2. Go access the above URL after having set "View | Character Set:
   Multibyte | Chinese Traditional (Big 5)".
3. Observe that the middle frame which contains pop-up menu does
   not show any of the menu items in Chinese.

If you access the middle frame directly under Big5 encoding,
Mozilla can display Chinese characters correctly:

http://us01.www.appledaily.com.hk/page/19991130/newm.htm

Comment 1

[karnaze (gone)]

Reassigning to Eric.

Comment 2

[Teruko Kobayashi]

I think this is related to bug 20315.

Comment 3

[Eric Pollmann]

Hehe, me too.  ;)  Perhaps you meant another bug number?

Comment 4

[Teruko Kobayashi]

Sorry, I meant the bug 20290

Comment 5

[Teruko Kobayashi]

I created the simple non-meta frame test case in http://babel/tests/browser/bft/sjis_no_meta/bft_frame_index_sjis.html
I tested this page in 112908 Win32 build.
After I visited this page and changed character set to Japanese (Shift_JIS), the Japanese characters are displayed as garbage.

I changed QA contact to [e-mail]

Comment 6

[Teruko Kobayashi]

cc to [e-mail].

Comment 7

[bobj]

This seems NOT related to bug 20290 because it occurs on non-frame pages.

Comment 8

[Katsuhiko Momoi]

I think we should keep this bug for the frame case and keep the other one for non-frame case.

Comment 9

[leger]

Putting on PDT+ radar.

Comment 10

[Eric Pollmann]

Marking M12 for the bug PDT+ push.

Comment 11

[Eric Pollmann]

I am taking a look at this bug now.  I've got Big5 fonts installed and working
on my Linux box.  Got started here:

http://www.mozilla.org/quality/intl/m11intlstatus.html

But the fonts are no longer at:
ftp://ftp.etl.go.jp/pub/mule/fonts/

and have moved to:
ftp://ftp.etl.go.jp/pub/mule/intlfonts-1.2-split/

Just grabbed Chinese.BIG.tar.gz
gunzip Chinese.BIG.tar.gz
cd intlfonts-1.2/Chinese.BIG
bdftopcf cc40s.bdf > cc40s.pcf
(repeat for each bdf file)
mkfontdir
xset +fp `pwd`

start apprunner, and away you go.  :)  Comboboxes showing Chinese fonts works
okay.  I am able to reproduce the bug and am taking a look.

Can anyone point me to what functions are called when
View->CharsetMultibyte->Big 5 is chosen?

Comment 12

[Eric Pollmann]

Ah... BrowserSetDefaultCharacterSet() in resources/content/navagator.js
Which does SetDocumentCharset on the appCore then reloads the page...

Comment 13

[Eric Pollmann]

Looking at our implementation of SetDocumentCharset in:
mozilla/xpfe/browser/src/nsBrowserInstance.cpp

It seems we are setting the charset on the top web shell, but not traversing to
get all of the child webshells and setting the charset on them as well.

Comment 14

[Eric Pollmann]

Oops, this is not the problem.  The child webshells have their charset set by
the document viewer.  My next best guess is that the reload destroys the child
webshells and recreates them, but does not pass default charset information down
to them on creation. (???)  Investigating this...

Comment 15

[Eric Pollmann]

I have confirmed the above.  I'm thinking of fixing this in
nsHTMLFrameInnerFrame.  After a webshell is created, I can get the parent
document viewer's default character set, then set the new child's document
viewer's character set to be the same.

Comment 16

[travis]

Is the charset not passed down when the child webshells are added to the parent
webshell?  Check out nsWebShell::AddChild.

Comment 17

[Eric Pollmann]

Ah, great!  Thanks for the pointer, I'll debug this.  :)

Comment 18

[Eric Pollmann]

Reassigning to Cata due to Frank Tang's suggestion.  There is a META charset
loading bug that is supposedly required to get this fixed...

Comment 19

[Eric Pollmann]

One thing I did notice was that in nsWebShell::AddChild, when the parent's
charset it supposed to be passed down to the child, the child does not have a
content viewer, so this code doesn't execute:

4559    nsCOMPtr<nsIContentViewer> childCV;
4560
NS_ENSURE_SUCCESS(childAsDocShell->GetContentViewer(getter_AddRefs(childCV)),
4561       NS_ERROR_FAILURE);
4562    if(childCV)   <<<<<<< childCV is null here so we don't set the charset
4563       {
4564       nsCOMPtr<nsIMarkupDocumentViewer> childmuDV =
do_QueryInterface(childCV);
4565       if(childmuDV)
4566          {
4567
NS_ENSURE_SUCCESS(childmuDV->SetDefaultCharacterSet(defaultCharset),
4568             NS_ERROR_FAILURE);
4569          NS_ENSURE_SUCCESS(childmuDV->SetForceCharacterSet(forceCharset),
4570             NS_ERROR_FAILURE);
4571          }
4572       }

Comment 20

[bobj]

Reassigned to cata, so clearing date until he has new estimate.

Comment 21

[Eric Pollmann]

I also noticed that in nsWebShell::Embed, where the new content viewer is
embeded in the web shell, it fails to get the charset "Big5" and set it on the
new content viewer.  This is because mChrsetReloadState == eCharsetReloadInit:

  if (mContentViewer && (eCharsetReloadInit!=mCharsetReloadState))
  { // get any interesting state from the old content viewer
    // XXX: it would be far better to just reuse the document viewer ,
    //      since we know we're just displaying the same document as before
... (get charset from old content viewer and set in new content viewer) ...

Because of this, in nsWebShell::AddChild, the charset that the parent is trying
to set on the child is "ISO-8895-1" instead of "Big5", as it should be.

I commented out the last half of this condition:

  if (mContentViewer) // && (eCharsetReloadInit!=mCharsetReloadState))
  { // get any interesting state from the old content viewer
    // XXX: it would be far better to just reuse the document viewer ,
    //      since we know we're just displaying the same document as before

This caused the new content viewer to get the old content viewer's charset
"Big5" in the reload.  Then in nsWebShell::AddChild, when it gets the charset it
get's the correct one.  We still have the problem that the child content viewer
is not created until after the AddChild call.

Perhaps the solution would be for the child to get it's parent's charset in the
child's Embed call?  This would ensure that the content viewer was already
created.  I'll try this out real quick and see if it does the trick...

Attaching a simplified test case...

Comment 22

[Eric Pollmann]

Attachment: inner frame, used by test case.

Comment 23

[Eric Pollmann]

Attachment: test case

Comment 24

[Eric Pollmann]

:p  Nope, that didn't work either.  It does set the child doc viewer's charset
to "Big5", but it's still displaying with the wrong font!

Here's the logic I tried for nsWebShell::Embed that didn't work (and may be
egregiously wrong, I don't know):

 835   nsCOMPtr<nsIContentViewer> oldContentViewer;
 836   if (mContentViewer)
 837   { // get any interesting state from the old content viewer
 838     oldContentViewer = mContentViewer;
 839   }
 840   else { // This is our first content viewer...
 841     // get any interesting state from our parent's content viewer
 842     nsCOMPtr<nsIWebShell> parent;
 843     nsresult res = GetParent(*getter_AddRefs(parent));
 844     if (NS_SUCCEEDED(res) && parent) {
 845       nsCOMPtr<nsIContentViewer> parentContentViewer;
 846       res = parent->GetContentViewer(getter_AddRefs(parentContentViewer));
 847       if (NS_SUCCEEDED(res)) {
 848         oldContentViewer = parentContentViewer;
 849       }
 850     }
 851   }
 852
 853   if (oldContentViewer) // && (eCharsetReloadInit!=mCharsetReloadState))
 854   {
 855     // XXX: it would be far better to just reuse the document viewer ,
 856     //      since we know we're just displaying the same document as before
... use oldContentViewer where mContentViewer was used below...

I'll attach a patch. It doesn't seem to solve the problem at hand, but I don't
know where to go from here.

Comment 25

[Eric Pollmann]

Attachment: patch (does not fix bug)

Comment 26

[buster]

looks like this got broken when I switched the charset data from webshell to
content viewer.  I did test this case, maybe something else changed too?

In any event, if the content viewer is not available at the time of
nsWebShell->AddChild, then we can do the same work (setting the various pieces
of charset data) to the time when the content viewer is hooked up to the
webshell.  The code that hooks the two objects up would just look at the parent
webshell and get the charset data from it.

Comment 27

[Frank Tang]

after apply the same debug patch (debug patch 1, and 2) in bug 21429, loading the
test cases produce the following-
From default charset, charset = ISO-8859-1
set to parser charset = ISO-8859-1 source 2
From default charset, charset =
set to parser charset =  source 2
From default charset, charset =
set to parser charset =  source 2

Notice the defaul charset for the inner frame is null

Comment 28

[Eric Pollmann]

Buster, if I'm reading code correctly, it looks like nsWebShell::Embed is the
routine that hooks up the content viewer and the webshell.  Is that right?

My patch (attached above) does what you suggest, but for some reason it's not
working.  In nsWebShell::Embed, when a child's content viewer is embeded, I made
it grab it's parent and get the charset from there.  I stepped through the code
and it appears to be working, but the font that's chosen is still wrong.  (Where
is the content viewer's charset used?)

I'll try the debug charset patch Frank mentions (bug 21429) to see what the
results are both before and after adding the changes to nsWebShell::Embed.

Comment 29

[Eric Pollmann]

Before and after my patch no change.  I did see the charset being set to Big5 in
both cases when I changed it using the View menu.

I did notice this though.  In nsHTMLDocument::StartDocumentLoad():
 538   nsCOMPtr<nsIMarkupDocumentViewer> muCV;
 539   nsCOMPtr<nsIContentViewer> cv;
 540   webShell->GetContentViewer(getter_AddRefs(cv));
 541   if (cv) {
 542      muCV = do_QueryInterface(cv);
 543   }

When we call webShell->GetContentViewer, it returns null but also returns
NS_OK.  This prevents us from getting a muCV and even though we're failing all
along here, we proceed to set the charset to null and mark the source as
kCharsetFromUserDefault.  The code that does this is not protected by the if
(muCV) as it apparently should be?

I also noticed that the code I hacked in nsWebShell::Embed is called AFTER
nsHTMLDocument::StartDocumentLoad, and I was assuming the opposite.  This means
that even though the charset gets passed down happily to the content viewer in
nsWebShell::Embed, it's too late because the parser and document have already
had their charset assigned the value "" (due to the aforementioned bug).

Perhaps we need to do all of this work in nsHTMLDocument::StartDocumentLoad.  If
we don't have an old content viewer in our webshell, get our parent's content
viewer and get the charset from there.  Is this reasonable?

This would make the charset code in nsWebShell::AddChild and nsWebShell::Embed
unneccessary, I think.  We also probably don't need the logic in
nsDocumentViewer that recurses through all of the children and sets their
charsets if we're just going to destroy them...

Comment 30

[Eric Pollmann]

Yes, this fixes the bug (and bug 21429 and possibly 21234 and 20290).  I'll post
a patch in a moment...

Comment 31

[Eric Pollmann]

Attachment: Patch part one (nsHTMLDocument)

Comment 32

[Eric Pollmann]

Attachment: Patch part two (nsWebShell)

Comment 33

[Eric Pollmann]

I'd love to get this reviewed and checked in this weekend.  Can someone take a
look and tell me if it is sane?  (Total bugfix = part one plus part two)

Comment 34

[buster]

I will review the fix tonight.  Sorry I couldn't get to it earlier, I was
unavailable for the last 2 days.

Comment 35

[buster]

The changes look great.  I applied them and tested the various related bugs and
everything seems to work (though my chinese is a little rusty.)  I'd talk to
frank before checking in, just to make sure he doesn't see any obvious holes
with the new strategy.  Also, I will attach a slightly modified version that
does a little better error checking and reporting.  The truth is, that whole
file needs a facelift with respect to error handling, but every little bit
helps.  You can accept it or not, in either event the code looks great.  Thanks
for tackling this.

Comment 36

[buster]

Attachment: some slight modifications to the way error handling is done in the patch

Comment 37

[Frank Tang]

steve- can you do a cvs -c ? It is not easy to fiture where you diff should go
by just the line number .... I am adding code to that file too...

Comment 38

[Frank Tang]

It looks like buster's patch and the nsWebShell.cpp changes fix the problem.
Great job. said r=ftang in your cvs check in log.

Comment 39

[Eric Pollmann]

Thanks guys!  I'm going to ask for approval using Steve's modified changes to
nsHTMLDocument in addition to the little fix in nsWebShell.

Comment 40

[Eric Pollmann]

Attachment: update nsHTMLDocument patch with -c

Comment 41

[Eric Pollmann]

Just checked in the fixes.

Comment 42

[Teruko Kobayashi]

I verified this in 121608 Win32, 121612 Linux and Mac build.

Comment 43

[Frank Tang]

<HTML>
 <FRAMESET COLS=*,*>
  <FRAME SRC="http://bugzilla.mozilla.org/showattachment.cgi?attach_id=3374">
  <FRAME SRC="http://bugzilla.mozilla.org/showattachment.cgi?attach_id=3375">
 </FRAMESET>
</HTML>

I have to reopen this bug. The check in fix the problem with 1 level depth frame
. But if the frame is another frameset, the problem is still exist. See the
above html. The first one is ok, but not the 2nd one.

Reopen and makr M13.

Comment 44

[Frank Tang]

*** Bug 22318 has been marked as a duplicate of this bug. ***

Comment 45

[Jim Roskind]

Removing the outdated entry on the whiteboard about "fix in hand" since this has
 reopened etc.

Comment 46

[Eric Pollmann]

Attachment: Test case #2

Comment 47

[Eric Pollmann]

I tested this with multiply nested framesets in one document e.g.

 <HTML>
  <FRAMESET>
    <FRAMESET>
    ...

And that works okay, it seems the problem is only if you have a frameset
doucment that contains a frame pointing to a frameset document e.g.

  <HTML>
   <FRAMESET>
    <FRAME SRC="foo.html">
  (where foo.html is a frameset document)

I'll take a look.

Comment 48

[Eric Pollmann]

I have a fix for this bug - it is to mirror the logic I added to
nsHTMLDocument::StartDocumentLoad to originally fix this bug to
nsWebShell::Embed

A summary of what was going wrong in Test case #2:

 Webshell A
     Webshell B (frame 1 in test case #2)
         Webshell C (frame containing chinese characters)
         Webshell (about:blank)
     Webshell (about:blank)

Before loading Webshell B ( StartDocumentLoad() ), we were getting the charset
information from it's parent Webshell A (Big 5) and handing it off to the
parser.  This is enough to cause any documents in Webshell B to be displayed
correctly.  Unfortunately, when we later created a content viewer for Webshell B
and attached it to a webshell ( Embed() ) we never copied down the parent
charset information from A to B.

Thus, when Webshell C is going to be loaded ( StartDocumentLoad() ), we get it's
parent charset information from Webshell B (ISO 8859-1) and pass that off to the
parser.  Since we never set the charset on Webshell B, it passes of the default
charset to the parser, and will display with ISO 8859-1 fonts.

The fix is to copy the charset information from A to the content viewer for B
when we create a content viewer for B (nsWebShell::Embed).

I'll attach my proposed patch.  It looks a lot like the patch to
nsHTMLDocument::StartDocumentLoad that was added earlier, including Steve's
debugging enhancements.

Comment 49

[Eric Pollmann]

Attachment: Patch to webshell (copy down charset from parent)

Comment 50

[Eric Pollmann]

Attachment: Patch to nsHTMLDocument (add a comment)

Comment 51

[buster]

changes look good.  r=buster.

Comment 52

[Eric Pollmann]

Thanks Steve!  I just checked in the fix.

1. Have Traditional Chinese fonts installed on your system.
2. Go access Test case #2  after having set "View | Character Set:
   Multibyte | Chinese Traditional (Big 5)".
3. Observe that the top left frame does show text in Chinese.

Thanks!

Comment 53

[Eric Pollmann]

I had to back this change out as it was causing some major regressions
(Javascript was not executing, event handlers were not executing).  Will try to
get it in later today if I can find a way to fix the problem...

Comment 54

[Brendan Eich [:brendan]]

*** Bug 24262 has been marked as a duplicate of this bug. ***

Comment 55

[Eric Pollmann]

I have a fix for this bug that does not cause the JS regression.

It is logically equivalent to the first fix, but makes sure that the old content
viewer is destroyed at the right time (it was a scoping issue of an nsCOMPtr
just as Tom Pixley suspected all along.)

Will attach for review momentarily.

Comment 56

[Eric Pollmann]

Attachment: Patch to webshell (same as above but without JS regression)

Comment 57

[buster]

r=buster (although I missed the regression on my last review, so trust me at
your peril!)

Comment 58

[Eric Pollmann]

Fix checked in.  Thanks!

Comment 59

[Teruko Kobayashi]

I verified this in 2000012008 Win32, Mac, and Linux build.