[webkit-reviews] review granted: [Bug 163412] REGRESSION: MessageEvent.source for bound postMessage() is wrong for same-origin iframe : [Attachment 307623] Patch and layout tests

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 163412: REGRESSION: MessageEvent.source for bound postMessage() is wrong
for same-origin iframe
https://bugs.webkit.org/show_bug.cgi?id=163412

Attachment 307623: Patch and layout tests

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




--- Comment #22 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 307623
  --> https://bugs.webkit.org/attachment.cgi?id=307623
Patch and layout tests

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

It seems like we have two names for this new kind of global object: "incumbent"
and "caller". I think we should try to use "incumbent" in as many places as
possible, if that's the spec name.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:306
> +		   return StackVisitor::Status::Done;

Seems like it would be better to continue in this case, and treat WASM like
bound functions, rather than giving up. Giving up will always result in a less
accurate result.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:336
> +	   // The currently running code was scheduled to run, say via
setTimeout(). Return the window
> +	   // associated with the code that scheduled us.

This comment is not the same as the comment in VM.h. I prefer the comment in
VM.h. I suggest removing this comment, and if there's something unique you like
about it, moving that to VM.h, so there's a single place where we explain the
concept of incumbentGlobalObject.

> Source/WebCore/bindings/js/ScheduledAction.cpp:135
> +    vm.setIncumbentGlobalObject(nullptr);

I wonder if you can make this automatic using something like WTF::SetForScope
or bmalloc::ScopeExit. This is a strong reference, so it would be nice to have
an automatic idiom for clearing it. You could also make a dedicated
IncumbentGlobalObjectScope class.