Bug 238747 - Use Ref and RefPtr pattern when handling document close calls
Summary: Use Ref and RefPtr pattern when handling document close calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gabriel Nava Marino
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-04 11:12 PDT by Gabriel Nava Marino
Modified: 2022-04-04 18:49 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.88 KB, patch)
2022-04-04 11:19 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2022-04-04 11:12:21 PDT
...
Comment 1 Gabriel Nava Marino 2022-04-04 11:19:43 PDT
Created attachment 456594 [details]
Patch
Comment 2 Sam Weinig 2022-04-04 11:30:36 PDT
Comment on attachment 456594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456594&action=review

> Source/WebCore/ChangeLog:8
> +        Ensure document object remains for the scope of the call.

I wonder ifs we should come up with some convention (naming or otherwise) to indicate that a caller is responsible for maintaining an external ref. Like, Document::close_mayDestroy(), or perhaps even take it a step further and require a separate type to call close()?  

Ref closer = document.closer()
closer.close()

/* can safely use document as long as closer is in scope */
Comment 3 Gabriel Nava Marino 2022-04-04 17:06:01 PDT
(In reply to Sam Weinig from comment #2)
> Comment on attachment 456594 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456594&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Ensure document object remains for the scope of the call.
> 
> I wonder ifs we should come up with some convention (naming or otherwise) to
> indicate that a caller is responsible for maintaining an external ref. Like,
> Document::close_mayDestroy(), or perhaps even take it a step further and
> require a separate type to call close()?  
> 
> Ref closer = document.closer()
> closer.close()
> 
> /* can safely use document as long as closer is in scope */

I agree this could be executed by explicit calls such above, which themselves could be enforced for example via assertions.

I can create a follow-up bug to track this suggestion.
Comment 4 Gabriel Nava Marino 2022-04-04 18:42:20 PDT
(In reply to Gabriel Nava Marino from comment #3)
> (In reply to Sam Weinig from comment #2)
> > Comment on attachment 456594 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=456594&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Ensure document object remains for the scope of the call.
> > 
> > I wonder ifs we should come up with some convention (naming or otherwise) to
> > indicate that a caller is responsible for maintaining an external ref. Like,
> > Document::close_mayDestroy(), or perhaps even take it a step further and
> > require a separate type to call close()?  
> > 
> > Ref closer = document.closer()
> > closer.close()
> > 
> > /* can safely use document as long as closer is in scope */
> 
> I agree this could be executed by explicit calls such above, which
> themselves could be enforced for example via assertions.
> 
> I can create a follow-up bug to track this suggestion.

https://bugs.webkit.org/show_bug.cgi?id=238780
Comment 5 EWS 2022-04-04 18:48:04 PDT
Committed r292361 (249226@main): <https://commits.webkit.org/249226@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456594 [details].
Comment 6 Radar WebKit Bug Importer 2022-04-04 18:49:16 PDT
<rdar://problem/91272528>