| Summary: | [WTF] Attach WARN_UNUSED_RETURN to makeScopeExit and fix existing wrong usage | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | Web Template Framework | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | achristensen, benjamin, cdumez, cmarcelo, darin, dbates, emilio, esprehn+autocc, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, rmorisset, rniwa, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 208171, 206343 | ||||||
| Attachments: |
|
||||||
|
Description
Yusuke Suzuki
2020-02-24 15:51:25 PST
Created attachment 391592 [details]
Patch
Comment on attachment 391592 [details]
Patch
r=me
Land it after ensuring all the EWS bots get green. Comment on attachment 391592 [details]
Patch
oops. r=me too
OK, building worked. Landing. Committed r257285: <https://trac.webkit.org/changeset/257285> 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! 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 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! 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! |