Bug 80427 – Crash dereferencing null pointer in DocumentWriter::addData

• Component: WebCore Misc.
• Status: REOPENED

[Raymes Khoury]

See http://code.google.com/p/chromium-os/issues/detail?id=21138

Some code is trying to call addData with a null-valued m_parser. There are 3 possible ways this could happen:
1) begin() has not been called before calling addData.
2) begin() has been called but the value of the parser is initialized to null.
3) addData() is called after end() has been called.

(3) seems most likely to me. Narrowing down which of these 3 cases is true will help to track down this bug and potentially others in the future.

[Raymes Khoury]

Created attachment 130401 [details]
Patch

[Nate Chapin]

Comment on attachment 130401 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130401&action=review

> Source/WebCore/loader/DocumentWriter.h:90
> +    enum WriterState {
> +        NotStartedWritingState,
> +        StartedWritingState,
> +        FinishedWritingState,
> +    };
> +    WriterState m_state;

A FIXME noting that this is temporary might be a good idea.

Also, since m_state is only read in ASSERTs, we should probably wrap this block and the places it's set in #ifndef NDEBUG blocks.

[Adam Barth]

Comment on attachment 130401 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130401&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

You won't be able to land this change without this line.  Perhaps replace it with a sentence that says this only adds ASSERTs and doesn't change any behavior?

[Raymes Khoury]

(In reply to comment #2)
> (From update of attachment 130401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130401&action=review
> 
> > Source/WebCore/loader/DocumentWriter.h:90
> > +    enum WriterState {
> > +        NotStartedWritingState,
> > +        StartedWritingState,
> > +        FinishedWritingState,
> > +    };
> > +    WriterState m_state;
> 
> A FIXME noting that this is temporary might be a good idea.
> 
> Also, since m_state is only read in ASSERTs, we should probably wrap this block and the places it's set in #ifndef NDEBUG blocks.

Just realized that I actually want CRASH() here rather than ASSERT(). What are the chances of that being OK'd?

[Adam Barth]

> Just realized that I actually want CRASH() here rather than ASSERT(). What are the chances of that being OK'd?

That's fine too.  Generally, we don't like to leave this sort of debugging code in the tree for long, but it can be helpful for tracking down these sorts of crashers.

[Raymes Khoury]

Created attachment 130407 [details]
Patch

[Raymes Khoury]

(In reply to comment #5)
> > Just realized that I actually want CRASH() here rather than ASSERT(). What are the chances of that being OK'd?
> 
> That's fine too.  Generally, we don't like to leave this sort of debugging code in the tree for long, but it can be helpful for tracking down these sorts of crashers.

Thanks. I added a FIXME to indicate that it should be changed to an ASSERT later.

[Raymes Khoury]

Created attachment 130411 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 130411 [details]
Patch

Attachment 130411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11836468

[Early Warning System Bot]

Comment on attachment 130411 [details]
Patch

Attachment 130411 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11833500

[Raymes Khoury]

Created attachment 130414 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130414 [details]
Patch

Clearing flags on attachment: 130414

Committed r109962: <http://trac.webkit.org/changeset/109962>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raymes Khoury]

Commit queue auto-closed.

[Alexey Proskuryakov]

It's been a month, are these CRASH checks catching anything?

[Nate Chapin]

(In reply to comment #15)
> It's been a month, are these CRASH checks catching anything?

It's given us some hints for debugging. I posted current theory at http://code.google.com/p/chromium/issues/detail?id=90693#c10

[Raymes Khoury]

Also see http://code.google.com/p/chromium-os/issues/detail?id=21138

[Alexey Proskuryakov]

See also: <rdar://problem/14080264>

[Joseph Pecoraro]

I am able to hit one of the CRASHes 100% right now with this test:

* TEST
<script>window.onunload = function() { debugger; }</script>

* STEPS TO REPRODUCE
1. Open test page
2. Open inspector
3. Reload
  => CRASH

[Myles C. Maxfield]

(In reply to comment #19)
> I am able to hit one of the CRASHes 100% right now with this test:
> 
> * TEST
> <script>window.onunload = function() { debugger; }</script>
> 
> * STEPS TO REPRODUCE
> 1. Open test page
> 2. Open inspector
> 3. Reload
>   => CRASH

This doesn't seem to occur any more on r201040