105971 - Navigator freezes when clicking a link which contains non-ASCII characters

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Koike Kazuhiko]

Navigator freezes when clicking a link which contains non-ASCII characters.

Testcase:
http://www1.mycal.co.jp/warnermycal/schedule/itabashi.html#20011020%A1%C120011026

Original report:
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=1509

Comment 1

[John Morrison]

stack trace for the hang:

copy_string(nsReadingIterator<char> & {...}, const nsReadingIterator<char> & 
{...}, CalculateUTF8Length & {...}) line 92 + 8 bytes
NS_ConvertUTF8toUCS2::Init(NS_ConvertUTF8toUCS2 * const 0x0012f994, const 
nsACString & {...}) line 1708
HTMLContentSink::ScrollToRef(HTMLContentSink * const 0x0012f994) line 4049 + 45 
bytes
HTMLContentSink::DidBuildModel(HTMLContentSink * const 0x02e89c68, int 
0x00000000) line 2759
CNavDTD::DidBuildModel(CNavDTD * const 0x02fa5c20, unsigned int 0x00000000, int 
0x00000001, nsIParser * 0x02f3c050, nsIContentSink * 0x021d7548) line 666 + 10 
bytes
nsParser::DidBuildModel(nsParser * const 0x0012f994, unsigned int 0x00000000) 
line 1381 + 13 bytes
nsParser::ResumeParse(nsParser * const 0x0012f994, int 0x00000001, int 
0x00000001) line 1890
nsParser::OnStopRequest(nsParser * const 0x02f3c054, nsIRequest * 0x02faf1d8, 
nsISupports * 0x00000000, unsigned int 0x00000000) line 2531
nsDocumentOpenInfo::OnStopRequest(nsDocumentOpenInfo * const 0x02f3c054, 
nsIRequest * 0x02faf1d8, nsISupports * 0x00000000, unsigned int 0x00000000) 
line 272
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x02fc1920, 
nsIRequest * 0x02faf1d8, nsISupports * 0x00000000, unsigned int 0x00000000) 
line 24 + 21 bytes
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x02faf1dc, nsIRequest * 
0x0234933c, nsISupports * 0x00000000, unsigned int 0x00000000) line 2326
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x0012f994) line 
177
PL_HandleEvent(PLEvent * 0x0231d6fc) line 591
PL_ProcessPendingEvents(PLEventQueue * 0x10031014) line 520 + 6 bytes
_md_EventReceiverProc(HWND__ * 0x00da4da8, unsigned int 0x004016d1, unsigned 
int 0x00d5d1f8, long 0x00000000) line 1071 + 10 bytes
nsAppShellService::Run(nsAppShellService * const 0x00d5d1f8) line 304
main1(int 0x00000002, char * * 0x00323c20, nsISupports * 0x00322ba8) line 1286 
+ 9 bytes
main(int 0x00000002, char * * 0x00323c20) line 1608 + 26 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x0013349e, 
HINSTANCE__ * 0x00400000) line 1626 + 21 bytes
MOZILLA! WinMainCRTStartup + 308 bytes
KERNEL32! 77e87903()

jag: I think this is your string changes from 10/16. A build from that morning
doesn't hang, but the current trunk does. 

(See also http://bugzilla.mozilla.org/show_bug.cgi?id=58819, for prior history)

Comment 2

[jag (Peter Annema)]

In a debug build I get:

ASSERTION: not a UTF-8 string.

Looking into this (we definitely shouldn't hang in such cases).

Comment 3

[jag (Peter Annema)]

Taking bug, changing component. teruko, I'm keeping you as the QA if you don't
mind.

Comment 4

[jag (Peter Annema)]

Here's where we're failing:
http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#4047

4047     // We assume that the bytes are in UTF-8, as it says in the spec:
4048     // http://www.w3.org/TR/html4/appendix/notes.html#h-B.2.1
4049     nsAutoString ref = NS_ConvertUTF8toUCS2(unescapedRef);

scc: I have a few options here to fix this:

1) don't use |copy_string|; instead, do something like:

  while (start != end) {
    size_t count = sink.write(start.get(), start.forward());
    if (count == 0)
      dealWithError();
    start.advance(count);
  }

2) track error state in sink, return |N|, to pretend we consumed all, on this
and any subsequent calls (which we'll not do anything with), and use the sink's
error state to base decisions off.

3) change copy_string to break on |0| instead of just asserting and somehow
forward to the copy_string call site that something went wrong.

Ideas? Suggestions? Alternatives?

Comment 5

[jag (Peter Annema)]

Oh, to clarify, the |CalculateUTF8Length| sink detects that the string passed in
isn't UTF8 and returns early. Since not all characters have been processed,
|copy_string| will call the sink again, starting at the place we just found to
not be UTF8, so the sink will return again, and |copy_string| will call the sink
again for the same place, ad infinitum.

Comment 6

[jag (Peter Annema)]

Alternatively (or additionally) we say that the code that depends on our current
(somewhat quirky) behaviour that NS_ConvertUTF8toUCS2 strings will be empty if
the string passed in wasn't UTF8 (spot the problem! :-) ) needs to be fixed.

We could for example export the failure somehow (I wish we could use
exceptions!), or provide a |IsUTF8(str)| scanning function. Again open for
suggestions.

Comment 7

[jag (Peter Annema)]

See bug 105904 for an attempt at fixing this (first patch).

Comment 8

[jag (Peter Annema)]

Fix checked in, marking so. You will get a warning/error message about trying to
convert a non-UTF8 string with NS_ConvertUTF8toUCS2 in debug builds.

Comment 9

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 10

[Yuying Long]

I can not reproduce it on 11-16 trunk build by click the links of orginal page,
mark it as verified.