197015 - Crash when loading table with CSS and typo [@ SinkContext::Begin ]

Status: VERIFIED FIXED

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

Description

[Dirk Heyne]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3) Gecko/20030311
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3) Gecko/20030311

When i try to search for a driver at http://www.intel.de (gets redirected to
http://www.intel.com/deutsch/) the 1.3 release candidate crashes, it is
reproduceable. It doesn't happen on the english site.

Reproducible: Always

Steps to Reproduce:
1. visit www.intel.de, get redirected to www.intel.com/deutsch/
2. type "1000 MT" into the search form and press enter
3. confirm submission -> crash

Actual Results:  
Mozilla crashed, talkback and Win98 error message came up

Expected Results:  
Mozilla should have displayed the search results

Talkback Incident IDs:
TB17987000G
TB17986958G
TB17986926Y

When i tried to reproduce the error, one time i got this error message:

Microsoft visual C++ Library
Runtime error!
Programm (...)MOZILLA.EXE
R6025
-pure virtual function call

that time mozilla didn't crash. I can't reproduce this, but perhaps it's a hint.
I'll attach a screenshot of mozilla frozen by the runtime error.

Could someone with deeper insight look if this is related to the gklayout Bugs
Bug 194582 or Bug 183220 ?

This is the message i get after a crash:

MOZILLA verursachte einen Fehler durch eine ungültige Seite
in Modul GKLAYOUT.DLL bei 0177:60320e5c.
Register:
EAX=020136bc CS=0177 EIP=60320e5c EFLGS=00010213
EBX=00000292 SS=017f ESP=0065ecf0 EBP=0065ed28
ECX=00000291 DS=017f ESI=02008d60 FS=4cd7
EDX=303d792e ES=017f EDI=00000000 GS=0000
Bytes bei CS:EIP:
8b 02 52 ff 50 40 eb 09 ff 75 fc 8b 02 52 ff 50 
Stapelwerte:
020a9400 00000291 00000000 00000000 020aa210 0200fe38 0200fd80 610bdf48 020a9400
00000000 00000000 020a9418 020a9418 020a9400 00000000 60322d03

Comment 1

[Dirk Heyne]

Attachment: mozilla frozen by runtime error

Comment 2

[Jesiah S]

It crashed on me too in Linux 2003031105 TB17995653X

Comment 3

[Torben]

Chrashes for me too, 20020310-1.3 (win95)

Comment 4

[Dirk Heyne]

Should this block the release of 1.3?
The visibility of this bug may be high:

Intel's Centrino marketing campaign just takes off,
and due to CeBit Fair Intel is frequently reported about,
e.g. at heise:
http://www.heise.de/newsticker/search.shtml?T=intel&thresh=1&hs=1

Comment 5

[Olivier Cahagne]

Attachment: Reduced testcase

Mozilla will not crash with the testcase if:
 - you remove the typo in the rel link to load the CSS htp -> http
 - you remove the <table> tag,
 - you remove the 'foo' content of the table.
Could be a parser issue, moving to Layout: Tables.
Sometimes, loading this testcase won't crash but will either hang or leave
Mozilla in memory after exiting.
Can someone test this with a 1.3 branch build ?

Comment 6

[Olivier Cahagne]

Moving to Layout: Tables.

Comment 7

[Olivier Cahagne]

(gdb) frame 1
#1  0x40f2ddb8 in HTMLContentSink::BeginContext (this=0x8974ed8, aPosition=1)
    at nsHTMLContentSink.cpp:2972
2972    ion].mNumFlushed,
(gdb) p content
$16 = (nsIHTMLContent *) 0x0
(gdb) p mCurrentContext->mStack[aPosition].mContent
$17 = (nsIHTMLContent *) 0x0
(gdb) p mCurrentContext->mStack[aPosition].mType
$18 = eHTMLTag_abbr
(gdb) frame 2
#2  0x415aeb4a in CNavDTD::HandleSavedTokens (this=0x8a4c3a8, anIndex=1)
    at CNavDTD.cpp:2100
2100            // The body context should contain contents only upto the marked
position.
(gdb) p theTag
$29 = eHTMLTag_table
(gdb) p theToken
$30 = (CToken *) 0x89600c8
(gdb) p attrCount
$31 = -1


#0  SinkContext::Begin (this=0x8a62270, aNodeType=eHTMLTag_abbr, aRoot=0x0,
    aNumFlushed=145087512, aInsertionPoint=-1) at nsHTMLContentSink.cpp:1457
#1  0x40f2ddb8 in HTMLContentSink::BeginContext (this=0x8974ed8, aPosition=1)
    at nsHTMLContentSink.cpp:2972
#2  0x415aeb4a in CNavDTD::HandleSavedTokens (this=0x8a4c3a8, anIndex=1)
    at CNavDTD.cpp:2100
#3  0x415ac69c in CNavDTD::HandleToken (this=0x8a4c3a8, aToken=0x8a512b8,
    aParser=0x8a60038) at CNavDTD.cpp:853
#4  0x415abc21 in CNavDTD::BuildModel (this=0x8a4c3a8, aParser=0x8a60038,
    aTokenizer=0x89602b8, anObserver=0x0, aSink=0x8974ed8) at CNavDTD.cpp:519
#5  0x415bcdab in nsParser::BuildModel (this=0x8a60038) at nsParser.cpp:1904
#6  0x415bcae7 in nsParser::ResumeParse (this=0x8a60038, allowIteration=1,
    aIsFinalChunk=0, aCanInterrupt=1) at nsParser.cpp:1771
#7  0x415bc024 in nsParser::ContinueParsing (this=0x8a60038)
    at nsParser.cpp:1388
#8  0x40f7317e in CSSLoaderImpl::SheetComplete (this=0x8a4ced8,
    aLoadData=0x8a627a8, aSucceeded=0) at nsCSSLoader.cpp:1459
#9  0x40f72753 in CSSLoaderImpl::LoadSheet (this=0x8a4ced8,
    aLoadData=0x8a627a8, aSheetState=eSheetNeedsParser) at nsCSSLoader.cpp:1298
#10 0x40f7403a in CSSLoaderImpl::LoadStyleLink (this=0x8a4ced8,
    aElement=0x8a62540, aURL=0x8a62660, aTitle=@0xbfffe4b0,
    aMedia=@0xbfffe390, aDefaultNameSpaceID=-1, aParserToUnblock=0x8a60038,
    aCompleted=@0xbfffe190, aObserver=0x0) at nsCSSLoader.cpp:1657
#11 0x410f57b9 in nsStyleLinkElement::UpdateStyleSheet (this=0x8a62568,
    aOldDocument=0x0, aObserver=0x0) at nsStyleLinkElement.cpp:335
#12 0x40f32fee in HTMLContentSink::ProcessLINKTag (this=0x8974ed8,
    aNode=@0x889c5d8) at nsHTMLContentSink.cpp:4985
#13 0x40f2ee6a in HTMLContentSink::AddLeaf (this=0x8974ed8, aNode=@0x889c5d8)
    at nsHTMLContentSink.cpp:3577
#14 0x40f2ed98 in HTMLContentSink::AddHeadContent (this=0x8974ed8,
    aNode=@0x889c5d8) at nsHTMLContentSink.cpp:3545
#15 0x415b1409 in CNavDTD::AddHeadLeaf (this=0x8a4c3a8, aNode=0x889c5d8)
    at CNavDTD.cpp:3807
#16 0x415ae22d in CNavDTD::HandleStartToken (this=0x8a4c3a8, aToken=0x8a50fb0)
    at CNavDTD.cpp:1779
#17 0x415acab0 in CNavDTD::HandleToken (this=0x8a4c3a8, aToken=0x8a50fb0,
    aParser=0x8a60038) at CNavDTD.cpp:950
#18 0x415abc21 in CNavDTD::BuildModel (this=0x8a4c3a8, aParser=0x8a60038,
    aTokenizer=0x89602b8, anObserver=0x0, aSink=0x8974ed8) at CNavDTD.cpp:519
#19 0x415bcdab in nsParser::BuildModel (this=0x8a60038) at nsParser.cpp:1904
#20 0x415bcae7 in nsParser::ResumeParse (this=0x8a60038, allowIteration=1,
    aIsFinalChunk=0, aCanInterrupt=1) at nsParser.cpp:1771
#21 0x415be242 in nsParser::OnDataAvailable (this=0x8a60038,
    request=0x8a4a6c0, aContext=0x0, pIStream=0x8a1382c, sourceOffset=0,
    aLength=176) at nsParser.cpp:2405
[...]

Comment 8

[Olivier Cahagne]

Harish, can you have a look at this bug ?

Comment 9

[Chris Casciano]

-> All/All (OS X 1.3RC)

TB ID: TB198025Z

Comment 10

[Moiz Dohadwala]

crashed when i followed the steps to reproduce.

win2k sp3, build 20030031216. talkback id: TB18023954K

Comment 11

[Andrew Schultz]

regression between linux trunk builds 2003010305 and 2003010404
bug 42945 and bug 182021 were checked in during that window.

Comment 12

[Andrew Schultz]

backing out bug 42945 (and bug 159615) didn't help.

Comment 13

[Dirk Heyne]

I tested with Build 2003010205 on Win95C. 

Both the Intel page and the testcase appear to work, 
but if i press control-Q afterwards Mozilla crashes reproduceable on exit  
"in Modul MORK.DLL bei 0137:61751b26".
Talkback IDs: TB18026302H and TB18025932X

Mozilla quitts as usual if i load an other Webpage between visiting
the Intel sides and quitting. - That was my first impression. Later
i discoverd, that Mozilla did NOT quitt but turned invisible, only
to get reactivated with the next start of a Mozilla version. Nasty.

It looks as if something was broken before this Bug emerged.

---

@ comment #5: The testcase crashes the 1.3 Branch Gecko/20030311
              on Win95C, too (TB18023547Q)

Talkback for an "original" crash at the intel side in Win95C: TB18026113K

Comment 14

[Boris Zbarsky [:bzbarsky]]

So... the issue is that we are calling ContinueParsing() on a parser that was
never blocked and in fact is sorta in the middle of a table.

Harish, should that lead to a crash?  I can fix the loader to not call
ContinueParsing() here if necessary, but I'd rather focus my time on not
blocking the parser at all....

Comment 15

[harishd]

Boris: The crash happens because of the lack of bounds checking in
Sink::AddComment. I will add that. 

Comment 16

[harishd]

It looks like the head context never got closed! 

Comment 17

[harishd]

>Harish, should that lead to a crash?  I can fix the loader to not call
>ContinueParsing() here if necessary, but I'd rather focus my time on not
>blocking the parser at all....

Boris: Could you please fix the loader? I don't understand what's the intention
of  calling ContinueParsing() given that the loader never blocks the parser (
AFAICT ). I'm assigning the bug to you for now. Feel free to bounce it back if
you think the bug does not belong to you.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Harish, the loader blocks the parser on every single non-alternate sheet that it
starts loading (and unblocks when the load completes).  But there are cases when
it could attempt to unblock without blocking first....

Perhaps my question should be rephrased as "do we need a fix for this in the 1.4
timeframe?"  Because in the 1.5 timeframe I plan to make the loader in fact not
block the parser....

Comment 19

[harishd]

>But there are cases whenit could attempt to unblock without blocking first....

That does not sound right IMO. In any case, fixing a crash is a good thing.
Let's put the band-aid and avoid the crash. You can fix it the way you wanted to
in 1.5.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Heh.  The band-aid will likely be a large patch itself... I'll try to get to it
for 1.4b, but not guarantees.  If it misses 1.4b, I'm not going to risk it for
1.4final.

Comment 21

[harishd]

Attachment: Band aid fix

Close the head context before the style sheet load is initiated. Also, added
bounds checking in AddComment. This fixes both the url and the reduced test
case.

Comment 22

[harishd]

Taking the bug to land the band aid fix. 

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 119024 [details] [diff] [review]
Band aid fix

Please also add a comment that this is a band aid fix for this bug and that it
could be removed once the real fix from Boris (bug no?) lands.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 119024 [details] [diff] [review]
Band aid fix

+  // Close the head context, that got opened up by the link tag, because
+  // our CSSLoader, on loading the linked style sheet, calls the parser's
+  // ContinueParsing() even if the parser is not  blocked and that causes 
+  // the sink, whose current context is head, to go haywire. 

Nit: Break up that really long sentence with way too many commas :-)

And add a reference to the bug we're bandaiding against here.

sr=jst

Comment 25

[harishd]

Hack landed. This shouldn't crash anymore. Reassigning the bug to Boris to fix
it the right way ;). Boris, make sure to test your patch with the hack removed.
Thanks!

Comment 26

[Dirk Heyne]

I tested with Build 2003040404 on Win98 SE (dt), 
and both the original page and the testcase work now,
no more crashes. Good work.

Boris: Should this Bug be marked FIXED or left open 
for your greater scale Fix?

Since we have a testcase, what about the intel page?
Many of the older builds out there will still crash (1.0x doesn't).
As their customer i could complain about their broken searchfunction,
or should tech evangelism contact intel to get the page fixed?
There are two other open tech evangelism bugs with intel, and i guess
a typo would be a good start if one argues that some pages are broken
on their site. (Bug 113099 and Bug 150555)

Comment 27

[Boris Zbarsky [:bzbarsky]]

See comment 25.  This should stay open, since I will be backing out the patch
Harish landed at some point....

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Since bug 220542 was checked in, there's a good chance this hack is no longer
needed. Stealing so I remember to check.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I've tested this testcase with the hack removed and we don't crash (it was
fixed by the patch for bug 220542). Looking for r= and sr= to back it out.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172842 [details] [diff] [review]
patch v1

sr=bzbarsky

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 172842 [details] [diff] [review]
patch v1

r=jst

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Marking this bug as fixed since the band aid was backed out (but the bug is
still fixed).

Comment 33

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using the testcase at
https://bugzilla.mozilla.org/attachment.cgi?id=117023 with build 2005-02-09-05
on Windows XP Seamonkey trunk.

Comment 34

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1