Bug 80427 - Crash dereferencing null pointer in DocumentWriter::addData
: Crash dereferencing null pointer in DocumentWriter::addData
Status: REOPENED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-06 10:10 PST by
Modified: 2014-03-13 16:39 PST (History)


Attachments
Patch (4.11 KB, patch)
2012-03-06 10:22 PST, Raymes Khoury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2012-03-06 11:03 PST, Raymes Khoury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2012-03-06 11:08 PST, Raymes Khoury
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.39 KB, patch)
2012-03-06 11:14 PST, Raymes Khoury
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-03-06 10:10:55 PST
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.
------- Comment #1 From 2012-03-06 10:22:37 PST -------
Created an attachment (id=130401) [details]
Patch
------- Comment #2 From 2012-03-06 10:29:34 PST -------
(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.
------- Comment #3 From 2012-03-06 10:30:43 PST -------
(From update of attachment 130401 [details])
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?
------- Comment #4 From 2012-03-06 10:44:03 PST -------
(In reply to comment #2)
> (From update of attachment 130401 [details] [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?
------- Comment #5 From 2012-03-06 10:46:34 PST -------
> 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.
------- Comment #6 From 2012-03-06 11:03:28 PST -------
Created an attachment (id=130407) [details]
Patch
------- Comment #7 From 2012-03-06 11:04:51 PST -------
(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.
------- Comment #8 From 2012-03-06 11:08:35 PST -------
Created an attachment (id=130411) [details]
Patch
------- Comment #9 From 2012-03-06 11:12:06 PST -------
(From update of attachment 130411 [details])
Attachment 130411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11836468
------- Comment #10 From 2012-03-06 11:13:45 PST -------
(From update of attachment 130411 [details])
Attachment 130411 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11833500
------- Comment #11 From 2012-03-06 11:14:18 PST -------
Created an attachment (id=130414) [details]
Patch
------- Comment #12 From 2012-03-06 14:45:27 PST -------
(From update of attachment 130414 [details])
Clearing flags on attachment: 130414

Committed r109962: <http://trac.webkit.org/changeset/109962>
------- Comment #13 From 2012-03-06 14:45:32 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 2012-03-14 12:32:57 PST -------
Commit queue auto-closed.
------- Comment #15 From 2012-04-14 10:47:35 PST -------
It's been a month, are these CRASH checks catching anything?
------- Comment #16 From 2012-04-16 08:56:18 PST -------
(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
------- Comment #17 From 2012-04-16 09:39:23 PST -------
Also see http://code.google.com/p/chromium-os/issues/detail?id=21138
------- Comment #18 From 2014-03-13 16:39:33 PST -------
See also: <rdar://problem/14080264>