95614 - parser crashes related to uninitialized iterators - Trunk [@ nsSharedBufferList::Position::Distance]

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Tor and I both saw parser crashes related to an uninitialized iterator in
CTextToken::Consume within a few minutes of starting to use today's build.  I'll
attach some snips of gdb output showing the problem.

Considering the frequency of the crash, this *may* need to be a smoketest
blocker, so marking as such at least until we have a better idea of how often it
happens.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: gdb output

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I suspect bug 91437 as the cause.

Comment 3

[R.K.Aa.]

it's a pretty bad one - smoketest blocker sounds fine. Had 6 of those crashes in
less than an hour.

Comment 4

[R.K.Aa.]

btw.. i ran with the "old" patch in bug 91437 for 3 days without problems
(attachment id=45504)

Comment 5

[harishd]

Does it happen only on LINUX?

Comment 6

[harishd]

I'm very close to a fix. Will attach a patch soon.

Comment 7

[harishd]

Attachment: patch v1.1 [ Initialize "end" iterator ]

Comment 8

[harishd]

Talked to vidur and realized that NS_ENSURE_SUCCESS will also assert ( a lot! )
and therefore would be more annoying than being helpful. Rewriting that part.

Comment 9

[vidur (gone)]

The old patch (attachment id=45504) uses the NS_ENSURE_SUCCESS macro which will
assert. A failure result code from nsScanner::Peek() does not indicate an
exceptional condition, just that we've reached the end of the current buffer.
Hence, an assertion is incorrect and the macro shouldn't be used.

Also, the early return in nsScanner::ReadUntil in the old patch will avoid the
call to Eof() at the end of the method. The original notion was that the scanner
could be used in push (the general case in the browser) or pull (sometimes used
in standalone tests) modes. I'm not sure if the latter mode is still functional,
but the call to Eof() was a crucial component of making it work correctly.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think there was another place that was very similar
(ConsumeMarkupDeclaration?) that should probably have the same fix...

Comment 11

[harishd]

Attachment: patch v1.2 [ addresses vidur's concerns ]

Comment 12

[harishd]

>I think there was another place that was very similar
>(ConsumeMarkupDeclaration?) that should probably have the same fix...

I don't think this code ever gets executed. Even if it does the scanner changes
in patch v1.2 should be able avoid the problem.

Comment 13

[harishd]

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

Comment 14

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

r=heikki, and sending to harishd

Comment 15

[Chris Waterson]

sr=waterson. you are so money, baby.

Comment 16

[harishd]

Marking FIXED.

Comment 17

[Blake Ross]

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

Comment 18

[Daniel Bratell]

Thanks for cleaning up my mess! 

I should have tested more and not just accepted other people's well intended 
advice.

Comment 19

[racham]

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

Comment 20

[Chase Tingley]

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

Comment 21

[Chase Tingley]

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

Comment 22

[Jay Patel [:jay]]

Adding crash, topcrash keywords and Trunk [@
nsSharedBufferList::Position::Distance] to summary for future reference.

Comment 23

[Moied]

dbaron, could you please verify this bug..?