329364 - Crash with iExploder testcase 10158270

Status: VERIFIED FIXED

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

Description

[Joonas]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060303 Firefox/1.6a1

The browser crashes with iExploder test 10158270

Found using http://toadstool.se/software/iexploder/

TB15912060Z, TB15932599H

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060228 Firefox/1.6a1 ID:2006022815

1. Go to http://toadstool.se/software/iexploder/
2. Enter 10158270 in 'Lookup a single test number:'
3. Press return or click lookup --> CRASH!

My TB15933186X [@ js_FindConstructor e0906f3a]

Comment 2

[Joonas]

Attachment: testcase

Reduced test case. TB15912033W, TB15929766H, TB15930568Z.

The source is <h1><table>a</h2><title>

Comment 3

[Joonas]

I think lots of iExploder crashes are variants of this bug. Tests 10073854, 10150163, 10158270, 10570989, 10707715 and 10797599 all look similar. For example 10073854 is essentially <h4><table>a</h5><title>, 10150163 is <h4><table>a</h2><style> etc.

Comment 4

[Martijn Wargers (dead)]

Seems like a parser bug to me.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Indeed it is.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Harish's fix for bug 25202 was not quite sufficient. His fix caught the case where the tag closing the context tag (which is the tag that we're inserting the misplaced content into) was the exact same as the context tag. In this case, however, we're looking at a quirk where </h2> closes the open <h1> tag, which is the "top" index. Therefore Harish's IndexOf call was returning the wrong answer, and we were closing the wrong context. This patch makes the HandleSavedTokens path imitate the HandleEndToken path, so it'll find the <h1> and discard the </h2> without doing any damage.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Also note that this patch might impose a small performance hit on pages that have malformed table content, but I'm hoping that it won't be large enough to notice (and since this is really badly malformed content, I don't think I care about penalizing such pages anyway).

Comment 8

[Martijn Wargers (dead)]

Does the patch also fix bug 329398 and bug 329399?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

bug 329398 is fixed by this patch, bug 329399 is not.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 214073 [details] [diff] [review]
Proposed fix

r+sr=jst

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better proposed fix

jst agrees with this fix on the fix, which is to avoid doing the LastOf call if we're unable to find a close target.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 214215 [details] [diff] [review]
Better proposed fix

Nominating for branches.

Comment 15

[Tim Riley [:timr]]

Comment on attachment 214215 [details] [diff] [review]
Better proposed fix

a=timr for drivers.  This fixes a blocker bug (329406)  that references this bug.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 17

[Dave Liebreich [:davel]]

oops - clicked wrong thing and marked bug as verified.  starting 2-step process to reset as resolved.

Comment 18

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060307 Firefox/1.5.0.2, no crash with iexploder test 10158270.

Comment 19

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

Verified FIXED on trunk using SeaMonkey build 2006-03-07-10 on Windows XP with the testcase of/at: https://bugzilla.mozilla.org/attachment.cgi?id=214022