Bug 80427 - Crash dereferencing null pointer in DocumentWriter::addData
Summary: Crash dereferencing null pointer in DocumentWriter::addData
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-06 10:10 PST by Raymes Khoury
Modified: 2017-03-18 16:07 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Raymes Khoury 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 Raymes Khoury 2012-03-06 10:22:37 PST
Created attachment 130401 [details]
Patch
Comment 2 Nate Chapin 2012-03-06 10:29:34 PST
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.
Comment 3 Adam Barth 2012-03-06 10:30:43 PST
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?
Comment 4 Raymes Khoury 2012-03-06 10:44:03 PST
(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?
Comment 5 Adam Barth 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 Raymes Khoury 2012-03-06 11:03:28 PST
Created attachment 130407 [details]
Patch
Comment 7 Raymes Khoury 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 Raymes Khoury 2012-03-06 11:08:35 PST
Created attachment 130411 [details]
Patch
Comment 9 Gustavo Noronha (kov) 2012-03-06 11:12:06 PST
Comment on attachment 130411 [details]
Patch

Attachment 130411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11836468
Comment 10 Early Warning System Bot 2012-03-06 11:13:45 PST
Comment on attachment 130411 [details]
Patch

Attachment 130411 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11833500
Comment 11 Raymes Khoury 2012-03-06 11:14:18 PST
Created attachment 130414 [details]
Patch
Comment 12 WebKit Review Bot 2012-03-06 14:45:27 PST
Comment on attachment 130414 [details]
Patch

Clearing flags on attachment: 130414

Committed r109962: <http://trac.webkit.org/changeset/109962>
Comment 13 WebKit Review Bot 2012-03-06 14:45:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Raymes Khoury 2012-03-14 12:32:57 PDT
Commit queue auto-closed.
Comment 15 Alexey Proskuryakov 2012-04-14 10:47:35 PDT
It's been a month, are these CRASH checks catching anything?
Comment 16 Nate Chapin 2012-04-16 08:56:18 PDT
(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 Raymes Khoury 2012-04-16 09:39:23 PDT
Also see http://code.google.com/p/chromium-os/issues/detail?id=21138
Comment 18 Alexey Proskuryakov 2014-03-13 16:39:33 PDT
See also: <rdar://problem/14080264>
Comment 19 Joseph Pecoraro 2015-09-15 09:32:35 PDT
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
Comment 20 Myles C. Maxfield 2016-05-17 16:03:27 PDT
(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