Closed Bug 121841 Opened 23 years ago Closed 22 years ago

<!ENTITY> hangs 0.9.8 and trunk using 100% CPU

Categories

(Core :: DOM: HTML Parser, defect, P2)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: jonasj, Assigned: jag+mozilla)

References

(
URL
)

Details

(Keywords: hang, regression, topembed+, Whiteboard: [ADT1] [ETA 04/16])

Attachments

(3 files, 2 obsolete files)

testcase
47 bytes, text/html
Details
Same fix, add comment
930 bytes, patch
bryner
: review+
scc
: superreview+
scc
: approval+
Details | Diff | Splinter Review
Fix that I just checked into the branch.
882 bytes, patch
Details | Diff | Splinter Review 
Build 2002012403, Win2k-SP2.

Go to http://www.tdc.dk/tdc/english/menu/sm0278.htm. Click one of the links
"Investor" or "Press" in the lower right corner of the page.
confirming withj win2k build 20020125..

I'm "looping" between this assertions:

###!!! ASSERTION: You can't |write| into an |nsWritingIterator| with no space!:
'size_forward() > 0', file ..\..\dist\include\string\nsStringIterator.h, line 35
###!!! ASSERTION: |copy_string| will never terminate: 'count_copied > 0', file .
.\..\dist\include\string\nsAlgorithm.h, line 91

I really don't know who should get this !
-> String ???????

The stack from the first assertion:
NTDLL! 778a018c()
nsDebug::Assertion(const char * 0x10132db8 `string', const char * 0x10132df8 
`string', const char * 0x10132c80 `string', int 356) line 290 + 13 bytes
nsWritingIterator<unsigned short>::write(const unsigned short * 0x01151e8c, 
unsigned int 75) line 356 + 37 bytes
nsCharSinkTraits<nsWritingIterator<unsigned short> 
>::write(nsWritingIterator<unsigned short> & {...}, const unsigned short * 
0x01151e8c, unsigned int 75) line 559
copy_string(nsReadingIterator<unsigned short> & {...}, const 
nsReadingIterator<unsigned short> & {...}, nsWritingIterator<unsigned short> & 
{...}) line 90 + 39 bytes
nsAString::do_AppendFromReadable(const nsAString & {...}) line 362 + 55 bytes
nsAString::AppendFromReadable(const nsAString & {...}) line 329
nsAString::Append(const nsAString & {...}) line 282 + 19 bytes
nsAutoString::nsAutoString(const nsAString & {...}) line 1455
nsCommentNode::SetText(nsCommentNode * const 0x0386bc00, const nsAString & 
{...}, int 0) line 246
nsGenericDOMDataNode::AppendData(const nsAString & {...}) line 508 + 50 bytes
nsCommentNode::AppendData(nsCommentNode * const 0x0386bc1c, const nsAString & 
{...}) line 60 + 18 bytes
SinkContext::AddComment(const nsIParserNode & {...}) line 1952
HTMLContentSink::AddComment(HTMLContentSink * const 0x03839720, const 
nsIParserNode & {...}) line 3473 + 18 bytes
CNavDTD::HandleCommentToken(CToken * 0x01124d08) line 2204 + 34 bytes
CNavDTD::HandleToken(CNavDTD * const 0x0383be10, CToken * 0x01124d08, nsIParser 
* 0x03839ac0) line 903 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x0383be10, nsIParser * 0x03839ac0, 
nsITokenizer * 0x038619c0, nsITokenObserver * 0x00000000, nsIContentSink * 
0x03839720) line 525 + 20 bytes
nsParser::BuildModel() line 1981 + 34 bytes
nsParser::ResumeParse(int 1, int 1, int 1) line 1847 + 11 bytes
nsParser::ContinueParsing() line 1495 + 19 bytes
nsParser::HandleParserContinueEvent() line 1555
nsParserContinueEvent::HandleEvent() line 230
HandlePLEvent(nsParserContinueEvent * 0x0386c790) line 241
PL_HandleEvent(PLEvent * 0x0386c790) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x004959b0) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00320250, unsigned int 49426, unsigned int 0, 
long 4807088) line 1071 + 9 bytes
USER32! 77e02e98()
USER32! 77e030e0()
USER32! 77e05824()
nsAppShellService::Run(nsAppShellService * const 0x004b5120) line 308
main1(int 2, char * * 0x00444fa0, nsISupports * 0x00000000) line 1285 + 32 bytes
main(int 2, char * * 0x00444fa0) line 1625 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87d08()
Assignee: asa → scc
Component: Browser-General → String
Keywords: hang
QA Contact: doronr → jaggernaut
CC dbaron because he can help in this case
The source of the page begins with:

<!ENTITY % HTML.Version "-//W3C//DTD HTML 4.01 Transitional//EN"
<html>
<head>
<title>Tele Danmark</title>

It seems to be the first line that causes it. Actually, it seems that even a
file containing only these 9 characters:

<!ENTITY>

will make Mozilla hang!
*** Bug 123723 has been marked as a duplicate of this bug. ***
It just became XP =)
OS: Windows 2000 → All
Summary: tdc.dk hangs build 2002012403 with 100% CPU → <!ENTITY> hangs 0.9.8 and trunk using 100% CPU
Keywords: regression
parser is the top caller culprit, cc'ing all victims and passing to parser.
Assignee: scc → harishd
Component: String → Parser
QA Contact: jaggernaut → moied
AFAIK, this is not a parser bug. Jag, could you please take a look at this bug
and see if it's string related?

--> m1.0
Priority: -- → P2
Target Milestone: --- → mozilla1.0
*** Bug 125827 has been marked as a duplicate of this bug. ***
As a note. I tried calling NS_ConvertUCS2toUTF8 on the string passed to
nsCommentNode::SetText.  After it chewed up about 600 MB of memory I killed it....

Something is wacky with the concatenation that's coming in there....


Mozilla doesn't hang if it sees ENTITY while rendering XML -
for example, the MathML team's demo pages (such as
http://www.mozilla.org/projects/mathml/demo/mfrac.xhtml) all have

  <!ENTITY mathml "http://www.w3.org/1998/Math/MathML">

at the top, and they render fine.  This with trunk 2002021508.

Investigating why XML mode makes a difference might be useful.

> Something is wacky with the concatenation that's coming in there....

Is something creating an nsPromiseConcatenation with the right side being
another nsPromiseConcatenation?
Not quite...  The left is is nsAutoString, the right side is
nsSlidingSubstring::nsAPromiseString
The case I described is known to fail (see bug 74709 / bug 70083).  I'm not sure
if what you described is the same.
jag, when I try this and pause the debugger when it seems to have hanged, I end
up in different locations but in some occasions it is showing nsSlidingSubstring
in the stack... so maybe more manifestations of the things we saw in bug 110544.
*** Bug 129739 has been marked as a duplicate of this bug. ***
hangs logging into www.verizon.com, so this affects ecommerce as well.
Keywords: topembed
*** Bug 130288 has been marked as a duplicate of this bug. ***
This is not a parser bug. Sounds like a string bug.

Observation:

<!XXXXXX> does not crash ( underlying string nsAFlatString )
<!ENTITY> does crash ( underlying string nsAPromiseString ).

--> jag
Assignee: harishd → jaggernaut
So it's either <!ENTITY> or nsPromiseFlatString or the combination.
*** Bug 131234 has been marked as a duplicate of this bug. ***
*** Bug 131785 has been marked as a duplicate of this bug. ***
*** Bug 132084 has been marked as a duplicate of this bug. ***
*** Bug 132354 has been marked as a duplicate of this bug. ***
Plusing to topembed+ since this is a regression and a hang.  Adding ADT1.
Keywords: topembed → topembed+
Whiteboard: [ADT1]
*** Bug 134020 has been marked as a duplicate of this bug. ***
*** Bug 134476 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.0+
*** Bug 134918 has been marked as a duplicate of this bug. ***
*** Bug 135495 has been marked as a duplicate of this bug. ***
Stack from bug 135495 (it is hang so got this hitting pause):

nsDependentConcatenation::GetReadableFragment(nsReadableFragment<unsigned short>
& {...}, nsFragmentRequest kFirstFragment, unsigned int 0) line 96 + 32 bytes
nsReadingIterator<unsigned short>::normalize_forward() line 371 + 39 bytes
nsReadingIterator<unsigned short>::advance(int 56) line 179
IsASCII(const nsAString & {...}) line 325 + 26 bytes
nsTextFragment::operator=(const nsAString & {...}) line 142 + 10 bytes
nsGenericDOMDataNode::SetText(nsGenericDOMDataNode * const 0x044509d8, const
nsAString & {...}, int 0) line 1330
nsGenericDOMDataNode::AppendData(const nsAString & {...}) line 508 + 50 bytes
nsCommentNode::AppendData(nsCommentNode * const 0x044509f4, const nsAString &
{...}) line 60 + 18 bytes
SinkContext::AddComment(const nsIParserNode & {...}) line 1978
HTMLContentSink::AddComment(HTMLContentSink * const 0x036bcb80, const
nsIParserNode & {...}) line 3503 + 18 bytes
CNavDTD::HandleCommentToken(CToken * 0x038cf848) line 2194 + 34 bytes
CNavDTD::HandleToken(CNavDTD * const 0x036cfeb0, CToken * 0x038cf848, nsIParser
* 0x0361a938) line 894 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x036cfeb0, nsIParser * 0x0361a938,
nsITokenizer * 0x038e6c10, nsITokenObserver * 0x00000000, nsIContentSink *
0x036bcb80) line 512 + 20 bytes
nsParser::BuildModel() line 1859 + 34 bytes
nsParser::ResumeParse(int 1, int 0, int 1) line 1731 + 11 bytes
nsParser::OnDataAvailable(nsParser * const 0x0361a93c, nsIRequest * 0x03726390,
nsISupports * 0x00000000, nsIInputStream * 0x036eca90, unsigned int 0, unsigned
int 1024) line 2383 + 21 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x03758000,
nsIRequest * 0x03726390, nsISupports * 0x00000000, nsIInputStream * 0x036eca90,
unsigned int 0, unsigned int 1024) line 242 + 46 bytes
nsUnknownDecoder::FireListenerNotifications(nsIRequest * 0x03726390, nsISupports
* 0x00000000) line 442 + 49 bytes
nsUnknownDecoder::OnDataAvailable(nsUnknownDecoder * const 0x03758178,
nsIRequest * 0x03726390, nsISupports * 0x00000000, nsIInputStream * 0x037ad308,
unsigned int 1024, unsigned int 3194) line 189 + 16 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x04456a60,
nsIRequest * 0x03726390, nsISupports * 0x00000000, nsIInputStream * 0x037ad308,
unsigned int 0, unsigned int 4218) line 242 + 46 bytes
nsFileChannel::OnDataAvailable(nsFileChannel * const 0x03726398, nsIRequest *
0x04456b14, nsISupports * 0x00000000, nsIInputStream * 0x037ad308, unsigned int
0, unsigned int 4218) line 507 + 49 bytes
nsOnDataAvailableEvent::HandleEvent() line 192 + 70 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x0445c0bc) line 116
PL_HandleEvent(PLEvent * 0x0445c0bc) line 596 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x01209048) line 526 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00120726, unsigned int 49458, unsigned int 0,
long 18911304) line 1077 + 9 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x011c91b8) line 309
main1(int 1, char * * 0x00304e90, nsISupports * 0x00000000) line 1350 + 32 bytes
main(int 1, char * * 0x00304e90) line 1698 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()
*** Bug 135988 has been marked as a duplicate of this bug. ***
note that any of the following will hang Mozilla:
<!ENTITY>  (this bug)
<!ELEMENT> (bug 127347)
<!ATTLIST>

Other tags with the same format do not seem to be a problem.
This seems like a very good bug to fix for 1.0, with lots people interested in
it. Can we get an ETA in Status Whiteboard?
Whiteboard: [ADT1] → [ADT1] [ETA ??/??]
High visibility with a lot of dupes. We really should try to get a fix into RC1.
Blocks: 134771
The problem here is in using nsSlidingSubstring as the second string of a 
dependent concatenation. The dependent concatenation, when asked for the next 
fragment, gets confused by the fragment the sliding substring returns (which 
isn't guaranteed to be the same as when the dependent concatenation was 
created), and concludes that the GetNextFragment call must've come for the first 
string (an empty nsAutoString in this case), so it'll return the next fragment 
relative to the first string, which is the first fragment in the sliding 
substring, while it should return the next fragment in the sliding substring (in 
this case, none). Net result: we keep iterating over the first fragment of the 
sliding substring ad infinitum.
*** Bug 137082 has been marked as a duplicate of this bug. ***
hmm. using linux trunk 2002041211 and it doesn't seem to hang anymore ...
Attached file testcase — 

    
    

    
  
the testcase does not hang. could anyone else try a recent build?

    
    

    
  
the testcase hangs with Linux build 2002041207 for me.  the URL removed the
offending tags, so it works even with older builds.

    
    

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

    
    

    
  
WFM with build 2002041203 on Win98SE with both the testcase, and the original URL.

    
    

    
  
So what changed? I tested a fair number of the dupes, and did not see any
problems. Finally one caused an assertion: can't write into writing iterator
with no space and that was an indication it went into a loop. In any case, this
is not completely gone but something has changed...

    
    

    
  
Branch still hangs. harishd tested that his line number fix did not change this
behavior.

    
  
Whiteboard: [ADT1] [ETA ??/??] → [ADT1] [ETA 10/16]

    
  
Whiteboard: [ADT1] [ETA 10/16] → [ADT1] [ETA 04/16]

    
    

    
  
Bug 77585 checkin seems like a good possibility for having partially fixed 
this.  The only other two checkins that even weakly touch this bug in the 
timeframe are bug 5693 and bug 133410.

    
    

    
  
I think comment 39 must have been incorrect.  CVS from 20020411 20:40 works. 

sorry for the confusion.

    
    

    
  
linux build 2002041108 works fine, 2002041008 hangs.

    
    

    
  
CVS-trunk 20020412 works.  I backed out the patch for bug 77585, and it hangs again.

    
    

    
  
Good find! I added a comment to the other bug about this.

Also those that are testing this: please test ALL the duplicates before you say
wfm. I saw at least one that was not fixed, and I did not test all dupes.

    
    

    
  
I tested all the dupes (except www.verizon.com, which needs an account) with
linux build 20020411 and they all worked.  Which one still didn't work for you?

(ac_gyrefalcon suggested bug 77585... I just tried it out)

    
    

    
  
Heikki-

If you've got a site that you think is still seeing this bug, check the char 
set.  If the node contains 2 byte chars or the concatenation involves non-ASCII 
it'll still use an nsAutoString which is what causes the blow up.

Should that be what's happening and jag's comment 34 is right then it's a maybe 
that checking for old_data being empty and avoiding the concatenation

    rv = SetText(old_data + aData, PR_FALSE);

on line 510 in that case would completely wallpaper over this bug for a quick 
fix.

And unfortunately I'm leaving for vacation in a few hours and won't find out 
how this progresses :(

    
    

    
  
It doesn't really matter whether old_data is empty. The point was that the code 
in nsDependentConcatenation gets confused by nsSlidingS(ubs)tring's behaviour of 
returning new fragment objects each time GetNextFragment is called.

The quick fix would be something along these lines:

    nsAutoString text(old_data);
    text.Append(aData);
    rv = SetText(text, PR_FALSE);

Though that'll do an extra copy. Would this be an issue?

    
    

    
  


  
We should really fix the underlying problem.

    
    

    
  
As a followup to comment #49. I just tested www.verizon.com (I have an account)
and it works fine on build 2002041203. So now I'm somewhat confused. If all
reported instances work with the latest builds, then what exactly is the patch
going to fix?

Or, is there an example URL that Andrew and I missed?

    
    

    
  
1) the patch for bug 77585 made it into the trunk and not the 1.0 branch.
2) the patch for bug 77585 only covers up the problem, rather than fixing it.

    
    

    
  


  
This patch plus the one for bug 77585 should fully cover up this problem by not

using the dependent concatenation.

    
    

    
  
Nice, I was merging this patch from an older version of this file where old_data
was later used for something else and couldn't be modified.

However, I think we should put such a hack, if we want it at all, at the call
site that passes in the SlidingSubstring. This code on its own is fine, it's the
passed in SlidingSubstring that's the problem, so IMO any work-around kinda
"fix" should be placed near there.

Of course, I'd rather look into real fix, which probably means reviving scc's
string fragment patch in bug 70083.

    
    

    
  
Comment on attachment 79220 [details] [diff] [review]
Same work around with no extra string copy

sr=scc.  Yes, let's work on the underlying problem from bug 70083; but this is
a reasonable and important patch in the meantime.

        Attachment #79220 -
        Flags: superreview+

    
    

    
  


  
I've changed my mind. The way I proposed would mean making sure all callers
were patched with the hack, while this keeps the hack local. Added a comment
with a reference to this bug.

        Attachment #79053 -
        Attachment is obsolete: true

        Attachment #79220 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 79393 [details] [diff] [review]
Same fix, add comment

r=bryner

        Attachment #79393 -
        Flags: review+

    
    

    
  
Comment on attachment 79393 [details] [diff] [review]
Same fix, add comment

sr=scc .. carrying forward my sr

        Attachment #79393 -
        Flags: superreview+

    
    

    
  
Comment on attachment 79393 [details] [diff] [review]
Same fix, add comment

a=scc (on behalf of drivers for mozilla1.0 .. after this vets on the trunk, I'm
sure ADT will want to plus this and get it on the branch ASAP)

        Attachment #79393 -
        Flags: approval+

    
    

    
  
Please checkin asap to branch, and add fixed1.0.0 keyword
Keywords: adt1.0.0+

    
    

    
  
Checked in to trunk and branch.
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED

    
    

    
  
Testcase and URL WFM with build 20020424 (branch) on WINXP, I have tested a fair
number of the dupes, and did not see any problems
Keywords: verified1.0.0

    
    

    
  
With 20020416, the browser does not hang, but my test page doesn't render quite
right either.  I see an "]>" at the top of the page, and the entity is not
replaced with its definition.  (This does work with an XHTML doctype.)

See <http://www.panix.com/~zackw/mozbugs/defining-entities.html>

Presumably this should be a separate bug, or has it already been reported?


    
    

    
  
That's a separate bug. heikki, is this a known bug or should one be filed?

    
    

    
  
Is that a regression? I am not sure we ever supported that, and IE6 does not
support it either.

    
    

    
  
It works if I change the doctype declaration to XHTML 1.0 -- the ]> disappears
and the entity reference is expanded.

I have no strong opinion as to whether plain-HTML documents should be allowed
to define local entities, but certainly the ]> should not appear.

    
    

    
  
Defining entities like that should not work in HTML. It works in XML, of course.

As far as showing ]> in HTML, all I want to know is if it is a regression from
some earlier Mozilla version. After that I will know if we need to open a new
bug or if I already have a bug on that.

    
    

    
  
Poking around through some old Usenet threads on DOCTYPE sniffing, M18
apparently used the same (dumb) heuristic we use today, which is to cruise
through looking for ">" and end the DOCTYPE there.

    
    

    
  
Doctype sniffing and parsing are two separate phases.  I wanted to unify them,
but the parser is too complicated.  So the DOCTYPE sniffing code is now smarter
than that, but the HTML parsing code is not.

    
    

    
  
Tested URL and testcase CPU is happy, Verified fixed with trunk build ID
20020529 using winxp
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: