Bug 237660

Summary: Main document is leaking on haaretz.co.il due to lazy image loading
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, bfulgham, darin, ews-watchlist, ggaren, japhet, rbuis, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=237619
https://bugs.webkit.org/show_bug.cgi?id=208094
https://bugs.webkit.org/show_bug.cgi?id=237672
Bug Depends on: 241921    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2022-03-09 08:58:04 PST 
Main document is leaking on haaretz.co.il due to lazy image loading.
Comment 1 Radar WebKit Bug Importer 2022-03-09 08:58:33 PST 
<rdar://problem/90035071>
Comment 2 Chris Dumez 2022-03-09 09:17:58 PST 
Created attachment 454248 [details]
Patch
Comment 3 Chris Dumez 2022-03-09 11:41:43 PST 
Created attachment 454273 [details]
Patch
Comment 4 Chris Dumez 2022-03-10 07:58:20 PST 
Created attachment 454362 [details]
Patch
Comment 5 Chris Dumez 2022-03-10 09:10:19 PST 
Created attachment 454368 [details]
Patch
Comment 6 Chris Dumez 2022-03-10 11:16:29 PST 
Created attachment 454381 [details]
Patch
Comment 7 Chris Dumez 2022-03-10 11:49:00 PST 
Created attachment 454387 [details]
Patch
Comment 8 Chris Dumez 2022-03-10 13:35:48 PST 
Created attachment 454394 [details]
Patch
Comment 9 Geoffrey Garen 2022-03-10 16:31:33 PST 
Comment on attachment 454394 [details]
Patch

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

r=me

> Source/WebCore/loader/ImageLoader.cpp:183
> +    auto inUpdateFromElementScope = makeScopeExit([this] {

I'm curious: Are you required to put a scope exit in a local variable?
Comment 10 Chris Dumez 2022-03-10 17:40:03 PST 
(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 454394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454394&action=review
> 
> r=me
> 
> > Source/WebCore/loader/ImageLoader.cpp:183
> > +    auto inUpdateFromElementScope = makeScopeExit([this] {
> 
> I'm curious: Are you required to put a scope exit in a local variable?

I don't understand the question, do you mean call makeScopeExit() and then not store the return value in a local variable? If so, definitely not and this is why makeScopeExit() is marked as WARN_UNUSED_RETURN. Something needs to control the lifetime of the "scope".

Also note that I usually would have used a simpler SetForScope<bool> for this. However, it turns out that it didn't work here because m_inUpdateFromElementScope is a bitfield (Apparently it is not valid C++ to have a reference to a bitfield data member).
Comment 11 EWS 2022-03-10 18:05:30 PST 
Committed r291141 (248301@main): <https://commits.webkit.org/248301@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454394 [details].
Comment 12 Geoffrey Garen 2022-03-11 10:12:30 PST 
> I don't understand the question, do you mean call makeScopeExit() and then
> not store the return value in a local variable? If so, definitely not and
> this is why makeScopeExit() is marked as WARN_UNUSED_RETURN. Something needs
> to control the lifetime of the "scope".

Got it.

> Also note that I usually would have used a simpler SetForScope<bool> for
> this. However, it turns out that it didn't work here because
> m_inUpdateFromElementScope is a bitfield (Apparently it is not valid C++ to
> have a reference to a bitfield data member).

Yeah, an unfortunate wart in the language.
Comment 13 Brent Fulgham 2022-05-26 14:54:11 PDT 
This fix shipped with Safari 15.5 (all platforms).