[webkit-reviews] review granted: [Bug 66783] fast/loader/document-destruction-within-unload.html causes an assertion failure in the next test (currently, document-with-fragment-url-1.html) : [Attachment 161584] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 66783: fast/loader/document-destruction-within-unload.html causes an
assertion failure in the next test (currently,
document-with-fragment-url-1.html)
https://bugs.webkit.org/show_bug.cgi?id=66783

Attachment 161584: patch
https://bugs.webkit.org/attachment.cgi?id=161584&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=161584&action=review


> Source/WebCore/ChangeLog:3
> +	   fast/loader/document-destruction-within-unload.html causes assertion
failures on mac and qt.

Is there a way to make a test that more directly targets this problem rather
than having the symptom be assertion failures in the next test that happen on
only certain platforms?

> Source/WebCore/loader/FrameLoader.cpp:1619
> +    // detachChildren() can trigger this frame's unload event, and can
therefore
> +    // do terrible, terrible things. For example, an unload event that calls


Whenever possible, I prefer more precise terminology than “can do terrible
things”. I want our comments to help people understand better, rather than
creating fear. So I’d be tempted to say something more like “therefore script
code from the webpage can run and do almost anything” or something like that.

> LayoutTests/ChangeLog:9
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

Can’t land a patch with this in it.