Bug 169189 – Replace debug assertion with release one in Frame::setView()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Replace debug assertion with release one in Frame::setView() to make make sure the document does not have a living render tree.

[Chris Dumez]

Created attachment 303486 [details]
Patch

[Chris Dumez]

I discussed this with Ryosuke and Gavin and they'd prefer to use a RELEASE_ASSERT here, so do I.

[Brent Fulgham]

I'm a little worried about this proposed change. We have crash trace reports that indicate that we do get into this state (though perhaps not after your recent changes). Is it really better to crash the user process than it is to do this cleanup?

CurrentlY:
1) If we never encounter this case, the cleanup code doesn't run.
2) If we do encounter this case, the cleanup code will put us back into a good state.

You are proposing we change to a user-visible crash, which doesn't seem like an obvious improvement to me.

[Ryosuke Niwa]

(In reply to comment #3)
> I'm a little worried about this proposed change. We have crash trace reports
> that indicate that we do get into this state (though perhaps not after your
> recent changes). Is it really better to crash the user process than it is to
> do this cleanup?
> 
> CurrentlY:
> 1) If we never encounter this case, the cleanup code doesn't run.
> 2) If we do encounter this case, the cleanup code will put us back into a
> good state.

Given that we don't believe we should ever get into this state, a release assert would be better. If we do get into this state, then we'd know from crash reports.

> You are proposing we change to a user-visible crash, which doesn't seem like
> an obvious improvement to me.

Given that this "cleanup" has caused regressions elsewhere, I'm not certain that crashing is necessarily worse.

[Chris Dumez]

(In reply to comment #3)
> I'm a little worried about this proposed change. We have crash trace reports
> that indicate that we do get into this state (though perhaps not after your
> recent changes). Is it really better to crash the user process than it is to
> do this cleanup?
> 
> CurrentlY:
> 1) If we never encounter this case, the cleanup code doesn't run.
> 2) If we do encounter this case, the cleanup code will put us back into a
> good state.
> 
> You are proposing we change to a user-visible crash, which doesn't seem like
> an obvious improvement to me.

My opinion is that if it still happens, we want to know sooner rather than later. A release assertion on trunk makes it way more likely for us to find possible remaining cases where this happens after my recent fix.

If it becomes too crashy, we can always roll out this patch to restore stability.

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #3)
> > I'm a little worried about this proposed change. We have crash trace reports
> > that indicate that we do get into this state (though perhaps not after your
> > recent changes). Is it really better to crash the user process than it is to
> > do this cleanup?
> > 
> > CurrentlY:
> > 1) If we never encounter this case, the cleanup code doesn't run.
> > 2) If we do encounter this case, the cleanup code will put us back into a
> > good state.
> > 
> > You are proposing we change to a user-visible crash, which doesn't seem like
> > an obvious improvement to me.
> 
> My opinion is that if it still happens, we want to know sooner rather than
> later. A release assertion on trunk makes it way more likely for us to find
> possible remaining cases where this happens after my recent fix.
> 
> If it becomes too crashy, we can always roll out this patch to restore
> stability.

Also note that it was easy to get the original test case to crash by modifying it slightly, despite the code reconstructing the render tree. If we arrive at this point with a render tree, it is still likely we'll crash despite reconstructing a render tree since we would be in a bad state and other places in the code do not handle this bad state.

[WebKit Commit Bot]

Comment on attachment 303486 [details]
Patch

Clearing flags on attachment: 303486

Committed r213521: <http://trac.webkit.org/changeset/213521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.