Regression(r268700) postMessage changes prototype of basic types

[Stefan Sechelmann]

Execute this in the current Safari Tech Preview (120):

>window.addEventListener('message', event => {
>  console.log('is Object after postMessage?: ', event.data instanceof Object)
>  console.log('is Array after postMessage?: ', event.data.array instanceof Array)
>})
>const testObject = {
>  array: [ 1, 2, 3, 4 ]
>};
>console.log('is Object before postMessage?: ', testObject instanceof Object)
>console.log('is Array before postMessage?: ', testObject.array instanceof Array)
>window.postMessage(testObject);

This probably extends to other build-in types like Map, Set, ArrayBuffer and the like...
This applies to Safari Tech Preview 120 and is not present in Safari 14.0.3.

[Stefan Sechelmann]

I experienced this previously when dealing with data read from IndexedDB in an iframe: bug 185906 and bug 182097.

[Stefan Sechelmann]

Here is a demo: https://codepen.io/sechel/pen/xxRLPKp
Check the dev console.

[Radar WebKit Bug Importer]

<rdar://problem/74612853>

[Yusuke Suzuki]

This is Structured Cloning related, so WebCore.

[Chris Dumez]

Indeed, I see this in STP 120:
[Log] is Object before postMessage?:  – true (iframeConsoleRunner-7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
[Log] is Array before postMessage?:  – true (iframeConsoleRunner-7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
[Log] is Object after postMessage?:  – false (iframeConsoleRunner-7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
[Log] is Array after postMessage?:  – false (iframeConsoleRunner-7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)

and this in my System Safari:
[Log] is Object before postMessage?:  – true (xxRLPKp, line 52)
[Log] is Array before postMessage?:  – true (xxRLPKp, line 53)
[Log] is Object after postMessage?:  – true (xxRLPKp, line 45)
[Log] is Array after postMessage?:  – true (xxRLPKp, line 46)

I will bisect.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> Indeed, I see this in STP 120:
> [Log] is Object before postMessage?:  – true
> (iframeConsoleRunner-
> 7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
> [Log] is Array before postMessage?:  – true
> (iframeConsoleRunner-
> 7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
> [Log] is Object after postMessage?:  – false
> (iframeConsoleRunner-
> 7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
> [Log] is Array after postMessage?:  – false
> (iframeConsoleRunner-
> 7f4d47902dc785f30dedcac9c996b9f31d4dfcc33567cc48f0431bc918c2bf05.js, line 1)
> 
> and this in my System Safari:
> [Log] is Object before postMessage?:  – true (xxRLPKp, line 52)
> [Log] is Array before postMessage?:  – true (xxRLPKp, line 53)
> [Log] is Object after postMessage?:  – true (xxRLPKp, line 45)
> [Log] is Array after postMessage?:  – true (xxRLPKp, line 46)
> 
> I will bisect.

Never mind, I am not actually seeing a recent regression. The difference I saw was before and after a reload.

If I load https://codepen.io/sechel/pen/xxRLPKp, I see:
[Log] is Object before postMessage?:  – true 
[Log] is Array before postMessage?:  – true 
[Log] is Object after postMessage?:  – true 
[Log] is Array after postMessage?:  – true 

If I press the reload button, I see:
[Log] is Object before postMessage?:  – true 
[Log] is Array before postMessage?:  – true 
[Log] is Object after postMessage?:  – false 
[Log] is Array after postMessage?:  – false 

I see this behavior in STP 120 as well as in older WebKit builds...

[Chris Dumez]

Ok, looks like I had not gone far back enough.
r267039: GOOD
r269041: BAD

In the good build, results are good even after the reload. It smells like a JSC bug to me but I guess we'll see after bisection.

[Chris Dumez]

Regression range:
https://trac.webkit.org/log/webkit/trunk?mode=follow_copy&rev=268700&stop_rev=268698

Almost certainly from http://trac.webkit.org/r268700, which I wrote :S

Investigating..

[Chris Dumez]

Seems it was caused by this part of the change:

JSC::JSGlobalObject* ScriptExecutionContext::globalObject()
{
-     if (is<Document>(*this)) {
-         Document& document = downcast<Document>(*this);
-         auto* frame = document.frame();
-         return frame ? frame->script().globalObject(mainThreadNormalWorld()) : nullptr;
-     }
+    if (is<Document>(*this))
+        return WebCore::globalObject(mainThreadNormalWorld(), downcast<Document>(*this).page());

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> Seems it was caused by this part of the change:
> 
> JSC::JSGlobalObject* ScriptExecutionContext::globalObject()
> {
> -     if (is<Document>(*this)) {
> -         Document& document = downcast<Document>(*this);
> -         auto* frame = document.frame();
> -         return frame ?
> frame->script().globalObject(mainThreadNormalWorld()) : nullptr;
> -     }
> +    if (is<Document>(*this))
> +        return WebCore::globalObject(mainThreadNormalWorld(),
> downcast<Document>(*this).page());

Oh, I think the issue is that WebCore::globalObject() ends up using the main frame's scriptController instead of the current frame's scriptController.

Will fix shortly.

[Chris Dumez]

I am struggling a bit to write a layout test that reproduces the issue. What seems to trigger the bug in Safari is to have Web Inspector open when the test is running.

[Chris Dumez]

Created attachment 421445 [details]
Patch

[Geoffrey Garen]

Comment on attachment 421445 [details]
Patch

r=me

[EWS]

Committed r273438: <https://commits.webkit.org/r273438>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421445 [details].

[Stefan Sechelmann]

Thank you.