Bug 237565 – REGRESSION (r290853): CrashTracer: com.apple.WebKit.WebContent.Development at com.apple.WebCore: WebCore::FrameLoader::clear

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kate Cheney]

REGRESSION (r290853): CrashTracer: com.apple.WebKit.WebContent.Development at com.apple.WebCore: WebCore::FrameLoader::clear

[Kate Cheney]

Created attachment 454048 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 454048 [details]
Patch

r=me, but might want an expert in FrameLoader code to review this as well.

[David Kilzer (:ddkilzer)]

<rdar://89923867>

[Chris Dumez]

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*.

[Kate Cheney]

Created attachment 454057 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Comment on attachment 454057 [details]
Patch

r=me with the change to Ref<>&&

[Kate Cheney]

(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.

[Kate Cheney]

(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>&&

[Kate Cheney]

Created attachment 454116 [details]
Patch

[Chris Dumez]

(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 :)

[EWS]

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].