Bug 237660 - Main document is leaking on haaretz.co.il due to lazy image loading
Summary: Main document is leaking on haaretz.co.il due to lazy image loading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 241921
Blocks:
  Show dependency tree / graph
 
Reported: 2022-03-09 08:58 PST by Chris Dumez
Modified: 2022-06-23 10:20 PDT (History)
11 users (show)

See Also:

Attachments
Patch (8.04 KB, patch)
2022-03-09 09:17 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (10.84 KB, patch)
2022-03-09 11:41 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2022-03-10 07:58 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (11.94 KB, patch)
2022-03-10 09:10 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (12.18 KB, patch)
2022-03-10 11:16 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.72 KB, patch)
2022-03-10 11:49 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.70 KB, patch)
2022-03-10 13:35 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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).