Null dereference in DOMSelection::deleteFromDocument

• Component: New Bugs
• Status: CLOSED FIXED

[Adam Barth]

Null dereference in DOMSelection::deleteFromDocument

[Adam Barth]

Created attachment 64671 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 64671 [details]
Patch

Clearing flags on attachment: 64671

Committed r65587: <http://trac.webkit.org/changeset/65587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

It's unfortunate we are landing this without a test case. I have run into similar circumstances many times before with fuzzers, crashes I understood fully but could not reproduce with a test case, and in the past I have typically struggled to make a test case for a day or two before landing without a test.

[Adam Barth]

We can revert the change if you like.  I have very little understanding of layout or editing.  I suspect that someone who understands these areas might be able to create a test case.

[Darin Adler]

(In reply to comment #5)
> We can revert the change if you like. I have very little understanding of layout or editing. I suspect that someone who understands these areas might be able to create a test case.

I don’t have a specific strategy to suggest to make sure a test case gets created. I don’t care who creates it. I would like to see someone add it to WebKit, though.

[Adam Barth]

Reopened for the test.

@enrica: any thoughts on who might understand this code well enough to write a test for this issue?

[Alexey Proskuryakov]

Adam, do you have a stack trace from this preserved somewhere?

[Adam Barth]

I didn't save the stack.  It was pretty boring.  This method is callable directly from JavaScript.  The trick is to get the DOM / render tree set up in such a way that updateLayout() makes the current selection disappear.  There's likely to be some trick to get that it happen, but I don't know enough about layout or selections to know how to set that up.

[Alexey Proskuryakov]

I don't know a lot about it either, but my guess is that one adds display:none to a style.

[Enrica Casucci]

One possible way to test this is:
1. create a selection
2. execCommand('indent'). This will move the selection under a block by cloning the selection under the new block and deleting the original one
3. call deleteFromDocument on the original selection

[Adam Barth]

That doesn't seem to work.  We need to make this condition true and have layout() blow away the selection.

    if (v && renderer() && (v->layoutPending() || renderer()->needsLayout()))
        v->layout();

[Adam Barth]

Created attachment 65734 [details]
Failed attempt at a test

[Alexey Proskuryakov]

I got this crash once, and added debug output to DOMSelection::deleteFromDocument(). If I hit this again, it will be easy to construct a test case. But it's quite rare.

[Eric Seidel (no email)]

I'm not sure this should be open any longer.  Our window for creating a test is gone.  Either this should be reverted or closed it seems.

[Alexey Proskuryakov]

Sad. Unfortunately, I never got this crash for the second time - but considering how easy it was to dump current selection state before it was lost for someone who could reproduce this, it's really wrong to not have a regression test.

[Ryosuke Niwa]

Created attachment 69103 [details]
failed attempt, hits other crashes

This is a hard one to reproduce.  I hit all sorts of other crashes before getting to this one.  Try replacing 'bold' by 'indent', 'insertorderedlist', etc...

[Eric Seidel (no email)]

(In reply to comment #17)
> Created an attachment (id=69103) [details]
> failed attempt, hits other crashes
> 
> This is a hard one to reproduce.  I hit all sorts of other crashes before getting to this one.  Try replacing 'bold' by 'indent', 'insertorderedlist', etc...

Please file bugs about the other crashes! :)  Those should be fixed too...

[Ryosuke Niwa]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=69103) [details] [details]
> > failed attempt, hits other crashes
> > 
> > This is a hard one to reproduce.  I hit all sorts of other crashes before getting to this one.  Try replacing 'bold' by 'indent', 'insertorderedlist', etc...
> 
> Please file bugs about the other crashes! :)  Those should be fixed too...

Sure I can file bugs.  But the real issue with these kind of crashes is our making a lot of assumptions after calling functions that modify DOM such as appendChild.  We ultimately need to make DOM mutation events asynchronous and don't fire it until we get out of composite editing commands.  Because it's really hard (or possibly impossible) for us to handle all possible modifications script can make at every stage of editing code.  I'm hoping to send out a proposal about this in a week or two.

[Koji Ishii]

In case you're still looking for a test case, I happened to come to this bug from bug 42844 and it looks like it has a reproducible script attached.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Let's add a test for this.