Summary: | WebAssembly: sending module to iframe fails | ||
---|---|---|---|
Product: | WebKit | Reporter: | JF Bastien <jfbastien> |
Component: | WebAssembly | Assignee: | 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, saam, webkit-bug-importer |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Attachments: |
Description
JF Bastien
2017-11-03 12:39:31 PDT
I've checked in Chrome and Firefox and they both pass this test. I'll look to this issue. (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 :-) (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 I think you're right, I talked to Chris Dumez and he agrees. 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. 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. Created attachment 328253 [details]
Patch
Patch uploaded
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 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 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 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 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 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 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 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
Created attachment 328262 [details]
Patch
Fixed tests
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 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
Looks like there are still 2 failures? Created attachment 328297 [details]
Patch
Skip wasm tests on ios simulator
(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 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. Created attachment 328441 [details]
Patch
Fix comments
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 on attachment 328441 [details]
Patch
LGTM too. Can you also add a test where you postMessage from an iframe to its parent window?
Created attachment 328628 [details]
Patch
Add one more test
Comment on attachment 328628 [details]
Patch
Update lgtm as well.
Comment on attachment 328628 [details] Patch Clearing flags on attachment: 328628 Committed r225656: <https://trac.webkit.org/changeset/225656> All reviewed patches have been landed. Closing bug. 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 Reverted r225656 for reason: The test has been a flaky timout since being added. Committed r226210: <https://trac.webkit.org/changeset/226210> 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 (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 Created attachment 330068 [details]
Patch
Fix flacky tests & checked with repeat 100 times
Comment on attachment 330068 [details]
Patch
r=me
(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 on attachment 330068 [details] Patch Clearing flags on attachment: 330068 Committed r226322: <https://trac.webkit.org/changeset/226322> All reviewed patches have been landed. Closing bug. |