Bug 208162 – [WTF] Attach WARN_UNUSED_RETURN to makeScopeExit and fix existing wrong usage

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yusuke Suzuki]

Fix r254739.

[Yusuke Suzuki]

Created attachment 391592 [details]
Patch

[Robin Morisset]

Comment on attachment 391592 [details]
Patch

r=me

[Yusuke Suzuki]

Land it after ensuring all the EWS bots get green.

[Saam Barati]

Comment on attachment 391592 [details]
Patch

oops. r=me too

[Yusuke Suzuki]

OK, building worked. Landing.

[Yusuke Suzuki]

Committed r257285: <https://trac.webkit.org/changeset/257285>

[Radar WebKit Bug Importer]

<rdar://problem/59747287>

[Darin Adler]

Comment on attachment 391592 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:305
> +            auto scopeExit = makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });

Seems like for this we should figure out what behavior was broken by not setting m_isHandlingBeforeLoad to true, and make a test that would detect that mistake!

[Yusuke Suzuki]

Comment on attachment 391592 [details]
Patch

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

>> Source/WebCore/html/HTMLLinkElement.cpp:305
>> +            auto scopeExit = makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
> 
> Seems like for this we should figure out what behavior was broken by not setting m_isHandlingBeforeLoad to true, and make a test that would detect that mistake!

Yeah, look like we are lacking the test for this case. To me, this looks like HTMLLinkElement is detached and reinserted during emitting before-load event is attached, but need some investigation.
Filed https://bugs.webkit.org/show_bug.cgi?id=208171

[Darin Adler]

Comment on attachment 391592 [details]
Patch

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

>>> Source/WebCore/html/HTMLLinkElement.cpp:305
>>> +            auto scopeExit = makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
>> 
>> Seems like for this we should figure out what behavior was broken by not setting m_isHandlingBeforeLoad to true, and make a test that would detect that mistake!
> 
> Yeah, look like we are lacking the test for this case. To me, this looks like HTMLLinkElement is detached and reinserted during emitting before-load event is attached, but need some investigation.
> Filed https://bugs.webkit.org/show_bug.cgi?id=208171

Oh, I forgot that this was the one that inspired this entire exercise!

[Yusuke Suzuki]

Comment on attachment 391592 [details]
Patch

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

>>>> Source/WebCore/html/HTMLLinkElement.cpp:305
>>>> +            auto scopeExit = makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
>>> 
>>> Seems like for this we should figure out what behavior was broken by not setting m_isHandlingBeforeLoad to true, and make a test that would detect that mistake!
>> 
>> Yeah, look like we are lacking the test for this case. To me, this looks like HTMLLinkElement is detached and reinserted during emitting before-load event is attached, but need some investigation.
>> Filed https://bugs.webkit.org/show_bug.cgi?id=208171
> 
> Oh, I forgot that this was the one that inspired this entire exercise!

The good thing is that, we also found this case in JSC Parser.cpp :). While it is only used for debug assertions, anyway, having this annotation is super useful!
Thanks Emilio!