REGRESSION (r290853): CrashTracer: com.apple.WebKit.WebContent.Development at com.apple.WebCore: WebCore::FrameLoader::clear
Created attachment 454048 [details] Patch
Comment on attachment 454048 [details] Patch r=me, but might want an expert in FrameLoader code to review this as well.
<rdar://89923867>
Comment on attachment 454048 [details] Patch I feel it would have been less risky to change the parameter to use a RefPtr<Document>&& instead of a Document*.
Created attachment 454057 [details] Patch
Comment on attachment 454057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454057&action=review > Source/WebCore/loader/FrameLoader.cpp:623 > +void FrameLoader::clear(RefPtr<Document> newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView, Function<void()>&& handleDOMWindowCreation) We should avoid passing parameters my values as much as possible. As suggested on Slack, I think this should be a Ref<Document>&&. > Source/WebCore/loader/FrameLoader.h:155 > + void clear(RefPtr<Document> newDocument, bool clearWindowProperties = true, bool clearScriptObjects = true, bool clearFrameView = true, Function<void()>&& handleDOMWindowCreation = nullptr); ditto.
(In reply to Chris Dumez from comment #6) > Comment on attachment 454057 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454057&action=review > > > Source/WebCore/loader/FrameLoader.cpp:623 > > +void FrameLoader::clear(RefPtr<Document> newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView, Function<void()>&& handleDOMWindowCreation) > > We should avoid passing parameters my values as much as possible. As > suggested on Slack, I think this should be a Ref<Document>&&. > > > Source/WebCore/loader/FrameLoader.h:155 > > + void clear(RefPtr<Document> newDocument, bool clearWindowProperties = true, bool clearScriptObjects = true, bool clearFrameView = true, Function<void()>&& handleDOMWindowCreation = nullptr); > > ditto. I looked at the call sites, it doesn't look like any can be updated to "move" the RefPtr in and avoid ref-counting churn. That said, I still think it is good practice to take in a Ref<Document>&& as it could potentially be leveraged in the future and there is no cost/drawback.
Comment on attachment 454057 [details] Patch r=me with the change to Ref<>&&
(In reply to Chris Dumez from comment #7) > (In reply to Chris Dumez from comment #6) > > Comment on attachment 454057 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=454057&action=review > > > > > Source/WebCore/loader/FrameLoader.cpp:623 > > > +void FrameLoader::clear(RefPtr<Document> newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView, Function<void()>&& handleDOMWindowCreation) > > > > We should avoid passing parameters my values as much as possible. As > > suggested on Slack, I think this should be a Ref<Document>&&. > > > > > Source/WebCore/loader/FrameLoader.h:155 > > > + void clear(RefPtr<Document> newDocument, bool clearWindowProperties = true, bool clearScriptObjects = true, bool clearFrameView = true, Function<void()>&& handleDOMWindowCreation = nullptr); > > > > ditto. > > I looked at the call sites, it doesn't look like any can be updated to > "move" the RefPtr in and avoid ref-counting churn. That said, I still think > it is good practice to take in a Ref<Document>&& as it could potentially be > leveraged in the future and there is no cost/drawback. Ah ok, that's why I ended up not using && but you're right about future leveraging.
(In reply to Chris Dumez from comment #6) > Comment on attachment 454057 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454057&action=review > > > Source/WebCore/loader/FrameLoader.cpp:623 > > +void FrameLoader::clear(RefPtr<Document> newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView, Function<void()>&& handleDOMWindowCreation) > > We should avoid passing parameters my values as much as possible. As > suggested on Slack, I think this should be a Ref<Document>&&. > Assuming you mean RefPtr<Document>&&
Created attachment 454116 [details] Patch
(In reply to Kate Cheney from comment #10) > (In reply to Chris Dumez from comment #6) > > Comment on attachment 454057 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=454057&action=review > > > > > Source/WebCore/loader/FrameLoader.cpp:623 > > > +void FrameLoader::clear(RefPtr<Document> newDocument, bool clearWindowProperties, bool clearScriptObjects, bool clearFrameView, Function<void()>&& handleDOMWindowCreation) > > > > We should avoid passing parameters my values as much as possible. As > > suggested on Slack, I think this should be a Ref<Document>&&. > > > > Assuming you mean RefPtr<Document>&& Totally :)
Committed r290994 (248172@main): <https://commits.webkit.org/248172@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454116 [details].