[webkit-reviews] review granted: [Bug 163412] REGRESSION: MessageEvent.source for bound postMessage() is wrong for same-origin iframe : [Attachment 309943] 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 309943: Patch and layout tests

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




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

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

r=me

> Source/JavaScriptCore/runtime/VM.cpp:467
> +JSGlobalObject* VM::topScriptHavingGlobalObject()

Let's use a shared helper function that we share with
CallFrame::callerSourceOrigin. It can return a CodeBlock* or nullptr.

Let's rename this to callerGlobalObject.

> Source/JavaScriptCore/runtime/VM.h:707
> +    // Global object of the "topmost entry of the JavaScript execution
context stack that has
> +    // a non-null ScriptOrModule component, or null if there is no such
entry in the JavaScript
> +    // execution context stack."
> +    //
<https://html.spec.whatwg.org/multipage/webappapis.html#topmost-script-having-e
xecution-context> (5 May 2017)
> +    JSGlobalObject* topScriptHavingGlobalObject();
> +
> +    void incrementSkipWhenDeterminingIncumbentCounter() {
++m_skipWhenDeterminingIncumbentCounter; }
> +    void decrementSkipWhenDeterminingIncumbentCounter()
> +    {
> +	   ASSERT(m_skipWhenDeterminingIncumbentCounter);
> +	   --m_skipWhenDeterminingIncumbentCounter;
> +    }
> +    bool shouldUseBackupIncumbentGlobalObject() const { return
m_skipWhenDeterminingIncumbentCounter > 0; }

This part of the algorithm is super crazy in two ways:

(1) Not all entry-points agree on whether they override the incumbent global
object on the JavaScript stack or not. For example, mutation event listeners do
override, but timers don't. That behavior is surprising and bad.

(2) It's never desirable -- for security or clarity -- to contradict what's
actually on the stack.

You should open a spec ticket to change this behavior. We want to always honor
the top of the JS stack if it's available.