Bug 179263

Summary: WebAssembly: sending module to iframe fails
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: WebAssemblyAssignee: GSkachkov <gskachkov>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, commit-queue, ews-watchlist, fpizlo, gskachkov, jfbastien, jlewis3, jsbell, keith_miller, mark.lam, msaboff, rmorisset, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch none

Description JF Bastien 2017-11-03 12:39:31 PDT
There's a WPT test here: https://github.com/w3c/web-platform-tests/blob/master/wasm/wasm_local_iframe_test.html


Which does the following:

<!DOCTYPE html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="resources/load_wasm.js"></script>
<iframe src="resources/frame.html" id="iframe"></iframe>
<script>
  promise_test(async function() {
    var mod = await createWasmModule();
    assert_true(mod instanceof WebAssembly.Module);
    var ans = await new Promise((resolve, reject) => {
      var iframe = document.getElementById("iframe").contentWindow;
      iframe.postMessage(mod, '*');
      window.addEventListener("message", (reply) => resolve(reply.data), false);
    });
    assert_equals(ans, 43);
  }, "send wasm module to iframe");
</script>


wpt.fyi says that we fail with:

Failure message: promise_test: Unhandled rejection with value: object "DataCloneError (DOM Exception 25): The object can not be cloned."



It seems like we should succeed? We can definitely postMessage to a Worker. I don't understand iframe enough to know why this fails, we should investigate.
Comment 1 GSkachkov 2017-12-01 00:53:51 PST
I've checked in Chrome and Firefox and they both pass this test. I'll look to this issue.
Comment 2 JF Bastien 2017-12-01 08:53:41 PST
(In reply to GSkachkov from comment #1)
> I've checked in Chrome and Firefox and they both pass this test. I'll look
> to this issue.

I understand that they pass, I'm just not sure whether they're right or we are and if so why :-)
Comment 3 GSkachkov 2017-12-01 13:11:47 PST
(In reply to JF Bastien from comment #2)
> (In reply to GSkachkov from comment #1)
> > I've checked in Chrome and Firefox and they both pass this test. I'll look
> > to this issue.
> 
> I understand that they pass, I'm just not sure whether they're right or we
> are and if so why :-)

Ohh. I see what you mean, I'm not expert in iframe :(  

But here some thoughts:

I think we also should allow postMessage to iframe because according to current spec:
```
A Module object is stateless and supports structured cloning which means that the compiled code can be stored in IndexedDB and/or shared between windows and workers via postMessage.
```

However by this https://github.com/WebKit/webkit/blob/master/Source/WebCore/bindings/js/SerializedScriptValue.cpp#L1069 we limited to post messages only to Workers.

Also as I can we see SharedArrayBuffer can be shared between window and iframe(https://html.spec.whatwg.org/multipage/webappapis.html#integration-with-the-javascript-agent-cluster-formalism):
```
SharedArrayBuffer objects can be shared.
...
A Window object A and the Window object of an iframe element that A created that could be same origin-domain with A.
```
So I would expect that we can do this for window and iframe for WebAssembly.Module
Comment 4 JF Bastien 2017-12-01 13:18:28 PST
I think you're right, I talked to Chris Dumez and he agrees.
Comment 5 Saam Barati 2017-12-01 13:20:20 PST
I also agree. The important thing here is we still prevent storing to indexDB since we don't have any way of doing that yet.
Comment 6 GSkachkov 2017-12-01 13:36:18 PST
Cool! I'll try to prepare fix for this. 

@Saam Barati, I see that we have test storage/indexeddb/resources/wasm-exceptions.js, that should protect us.
Comment 7 GSkachkov 2017-12-02 14:01:54 PST
Created attachment 328253 [details]
Patch

Patch uploaded
Comment 8 EWS Watchlist 2017-12-02 15:01:16 PST
Comment on attachment 328253 [details]
Patch

Attachment 328253 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5469942

New failing tests:
workers/sab/postMessage-clones.html
Comment 9 EWS Watchlist 2017-12-02 15:01:17 PST
Created attachment 328256 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 EWS Watchlist 2017-12-02 15:13:16 PST
Comment on attachment 328253 [details]
Patch

Attachment 328253 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5469954

New failing tests:
workers/sab/postMessage-clones.html
Comment 11 EWS Watchlist 2017-12-02 15:13:18 PST
Created attachment 328257 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 EWS Watchlist 2017-12-02 15:38:03 PST
Comment on attachment 328253 [details]
Patch

Attachment 328253 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5469975

New failing tests:
workers/sab/postMessage-clones.html
Comment 13 EWS Watchlist 2017-12-02 15:38:05 PST
Created attachment 328259 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 EWS Watchlist 2017-12-02 15:39:26 PST
Comment on attachment 328253 [details]
Patch

Attachment 328253 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5469992

New failing tests:
wasm/iframe-postmessage.html
workers/sab/postMessage-clones.html
wasm/window-postmessage.html
Comment 15 EWS Watchlist 2017-12-02 15:39:28 PST
Created attachment 328260 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 16 GSkachkov 2017-12-02 15:53:17 PST
Created attachment 328262 [details]
Patch

Fixed tests
Comment 17 EWS Watchlist 2017-12-02 17:31:29 PST
Comment on attachment 328262 [details]
Patch

Attachment 328262 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5470679

New failing tests:
wasm/iframe-postmessage.html
wasm/window-postmessage.html
Comment 18 EWS Watchlist 2017-12-02 17:31:31 PST
Created attachment 328268 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 19 JF Bastien 2017-12-02 21:20:59 PST
Looks like there are still 2 failures?
Comment 20 GSkachkov 2017-12-03 11:03:09 PST
Created attachment 328297 [details]
Patch

Skip wasm tests on ios simulator
Comment 21 GSkachkov 2017-12-03 11:05:10 PST
(In reply to JF Bastien from comment #19)
> Looks like there are still 2 failures?

Yeah, it appears that wasm does not work correctly on --ios-simulator.
Comment 22 JF Bastien 2017-12-04 16:55:26 PST
Comment on attachment 328297 [details]
Patch

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

Looks good other than the iOS tests.

> LayoutTests/platform/ios/TestExpectations:2909
> +wasm/window-postmessage.html [ Skip ]

This seems wrong, no? We only want to skip on iOS simulator because wasm requires JIT, but not on iOS itself.
Comment 23 GSkachkov 2017-12-04 23:03:43 PST
Created attachment 328441 [details]
Patch

Fix comments
Comment 24 JF Bastien 2017-12-04 23:35:13 PST
Comment on attachment 328441 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:8
> +        Allow use WebAssembly.Module as input paramters for postMessage 

Typo "paramters"
Comment 25 Saam Barati 2017-12-05 09:51:27 PST
Comment on attachment 328441 [details]
Patch

LGTM too. Can you also add a test where you postMessage from an iframe to its parent window?
Comment 26 GSkachkov 2017-12-06 14:25:12 PST
Created attachment 328628 [details]
Patch

Add one more test
Comment 27 JF Bastien 2017-12-07 09:19:56 PST
Comment on attachment 328628 [details]
Patch

Update lgtm as well.
Comment 28 WebKit Commit Bot 2017-12-07 16:56:23 PST
Comment on attachment 328628 [details]
Patch

Clearing flags on attachment: 328628

Committed r225656: <https://trac.webkit.org/changeset/225656>
Comment 29 WebKit Commit Bot 2017-12-07 16:56:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2017-12-07 17:03:45 PST
<rdar://problem/35923865>
Comment 31 Matt Lewis 2017-12-20 18:07:58 PST
The following layout test is a flaky timeout on macOS:

wasm/iframe-postmessage.html

Probable cause:

Most likely flaky from the time of being added in revision
r225656 https://trac.webkit.org/changeset/225656/webkit

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=wasm%2Fiframe-postmessage.html

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r226203%20(2204)/results.html

--- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/wasm/iframe-postmessage-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/wasm/iframe-postmessage-actual.txt
@@ -1,3 +1,4 @@
+FAIL: Timed out waiting for notifyDone to be called
 Test that expected Wasm Module can be sent over iframe.postMessage.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
@@ -6,5 +7,4 @@
 PASS successfullyParsed is true
 
 TEST COMPLETE
-PASS () => value is 35010
Comment 32 Matt Lewis 2017-12-20 18:38:06 PST
Reverted r225656 for reason:

The test has been a flaky timout since being added.

Committed r226210: <https://trac.webkit.org/changeset/226210>
Comment 33 GSkachkov 2017-12-20 23:35:37 PST
Ohh, I'll look to the issue.

I think cause of flakiness is that I did wrong assumption and I use setTimeout in test, but in many cases iframe is not ready in time when we send postMessage :(
I'll try to release fix
Comment 34 GSkachkov 2017-12-20 23:48:50 PST
(In reply to Matt Lewis from comment #32)
> Reverted r225656 for reason:
> 
> The test has been a flaky timout since being added.
> 
> Committed r226210: <https://trac.webkit.org/changeset/226210>

I could not find iframe-postMessage.html in rollback patch, that fails in tests
Comment 35 GSkachkov 2017-12-21 14:19:06 PST
Created attachment 330068 [details]
Patch

Fix flacky tests & checked with repeat 100 times
Comment 36 JF Bastien 2018-01-02 06:37:52 PST
Comment on attachment 330068 [details]
Patch

r=me
Comment 37 GSkachkov 2018-01-02 07:30:58 PST
(In reply to JF Bastien from comment #36)
> Comment on attachment 330068 [details]
> Patch
> 
> r=me
Thanks!
I'll keep my eye on test 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=wasm%2Fiframe-postmessage.html
Comment 38 WebKit Commit Bot 2018-01-02 07:38:44 PST
Comment on attachment 330068 [details]
Patch

Clearing flags on attachment: 330068

Committed r226322: <https://trac.webkit.org/changeset/226322>
Comment 39 WebKit Commit Bot 2018-01-02 07:38:47 PST
All reviewed patches have been landed.  Closing bug.