Null dereference in DOMSelection::deleteFromDocument
Created attachment 64671 [details] Patch
Comment on attachment 64671 [details] Patch Clearing flags on attachment: 64671 Committed r65587: <http://trac.webkit.org/changeset/65587>
All reviewed patches have been landed. Closing bug.
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.
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.
(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.
Reopened for the test. @enrica: any thoughts on who might understand this code well enough to write a test for this issue?
Adam, do you have a stack trace from this preserved somewhere?
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.
I don't know a lot about it either, but my guess is that one adds display:none to a style.
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
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();
Created attachment 65734 [details] Failed attempt at a test
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.
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.
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.
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...
(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...
(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.
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.
*** Bug 42844 has been marked as a duplicate of this bug. ***
Let's add a test for this.