[webkit-reviews] review granted: [Bug 169855] Crash when breakpoint hit in unload handler : [Attachment 333662] patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 169855: Crash when breakpoint hit in unload handler
https://bugs.webkit.org/show_bug.cgi?id=169855

Attachment 333662: patch

https://bugs.webkit.org/attachment.cgi?id=333662&action=review




--- Comment #5 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 333662
  --> https://bugs.webkit.org/attachment.cgi?id=333662
patch

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

Looks sane to me.

r=me

> Source/WebCore/ChangeLog:11
> +	   CachedRawResource::updateBuffer may generate unload event in client
notify callback. If Inspector was

CachedRawResource::updateBuffer => updateBuffer()

> Source/WebCore/ChangeLog:12
> +	   paused, this even would spawn a nested runloop.
CachedRawResource::finishLoading would get called in

CachedRawResource::finishLoading => CachedRawResource::finishLoading()

> Source/WebCore/ChangeLog:20
> +	   Set a bit when entering the client callback.
> +	   Ensure we don't re-enter updateBuffer.
> +	   If finishLoading got delayed during client callback, do it at the
end.

This is OK as-is. I do not see the need to put each sentence on its own line. I
suggest you removing the newlines and use the same approach as you did when
writing the description for this ChangeLog and break long lines around 100
characters.

> Source/WebCore/loader/cache/CachedRawResource.cpp:59
> +    // Skip any updateBuffers triggered from nested runloops. We'll have
complete buffer in finishLoading.

have => have the
updateBuffers => updateBuffers()
finishLoading => finishLoading()

> Source/WebCore/loader/cache/CachedRawResource.cpp:84
> +	   auto buffer = WTFMove(m_delayedFinishLoadingData->buffer);
> +	   m_delayedFinishLoadingData = std::nullopt;
> +	   finishLoading(buffer.get());

I would have written this as:

auto delayedFinishLoadingData = std::exchange(m_delayedFinishLoadingData,
std::nullopt);
finishLoading(delayedFinishLoadingData->buffer.get());

> Source/WebCore/loader/cache/CachedRawResource.h:96
> +    struct DelayedFinishLoadingData {
> +	   RefPtr<SharedBuffer> buffer;
> +    };

I take it you feel that this struct improves the readability of the code or
makes the code less error prone as opposed to using
std::optional<RefPtr<SharedBuffer>> or defining DelayedFinishLoadingData to be
a type alias for RefPtr<SharedBuffer>.