On iOS 10, a `postMessage` from This is a change from iOS 9.
[Sorry, early send.] On iOS 10 Safari, a `postMessage` from an iframe with the same origin as the parent window results in a MessageEvent with the `.source` property set to the parent window. It should be set to the window of the message source. This is a behavior change from iOS 9. P.S.: Because `.source` is scrubbed, if the parent has two child frames with the parent's origin, there's no data on the event object which could distinguish one frame's messages from the other's.
<rdar://problem/28776024>
(In reply to comment #1) > [Sorry, early send.] > > On iOS 10 Safari, a `postMessage` from an iframe with the same origin as the > parent window results in a MessageEvent with the `.source` property set to > the parent window. It should be set to the window of the message source. > > This is a behavior change from iOS 9. > > P.S.: Because `.source` is scrubbed, if the parent has two child frames with > the parent's origin, there's no data on the event object which could > distinguish one frame's messages from the other's. Do you have a test case / example?
Created attachment 297469 [details] Example Attached page that demonstrates this issue. Running the example in Safari version Version 10.0 (12602.1.50.0.10) I see "Got message from A: hello". In Chrome for Mac version 56.0.2924.28 beta (64-bit) and Firefox for Mac version 49.0.1 I see "Got message from B: hello". The expected result should be the latter, "Got message from B: hello".
Created attachment 304301 [details] Patch and layout tests
Attachment 304301 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 304309 [details] Patch and layout tests Add forwarding header file for JSBoundFunction.h.
Attachment 304309 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/runtime/JSBoundFunction.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
From talking with Geoffrey Garen on or around 03/16/2017 about bug 162075, Geoff felt the implementation of callerDOMWindow() even after the changes made in attachment 304309 [details] has correctness issues with respect to builtin functions and pointed out that we can stop walking the stack once we hit a ScriptExecutable.
Created attachment 307546 [details] Patch and layout tests Modified the implementation of callerDOMWindow() to use a similiar approach as ExecState::callerSourceOrigin(). This work was inspired by a patch by Geoffrey Garen.
Attachment 307546 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 307546 [details] Patch and layout tests Attachment 307546 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3567686 New failing tests: imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
Created attachment 307556 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307546 [details] Patch and layout tests Attachment 307546 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3567933 New failing tests: imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
Created attachment 307563 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307546 [details] Patch and layout tests Attachment 307546 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3568219 New failing tests: imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
Created attachment 307570 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307546 [details] Patch and layout tests Attachment 307546 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3568930 New failing tests: imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
Created attachment 307583 [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
Created attachment 307623 [details] Patch and layout tests Modified WebCore::callerDOMWindow() to return the first DOM window if did not find non-native code in the stack trace and the currently running code was not scheduled (say, via setTimeout()). This fixes the assertion failure and makes another subtest pass in LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html. This also preserves our current behavior of returng the first DOM window as the calling DOM window should WebCore::callerDOMWindow() be invoked when running Wasm code (see bug #165721).
Attachment 307623 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 307623 [details] 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.
Comment on attachment 307623 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=307623&action=review > Source/WebCore/bindings/js/ScheduledAction.cpp:130 > + vm.setIncumbentGlobalObject(m_callerGlobalObject.get()); I would assert it's nullptr here before setting the value, otherwise, the code below that sets it back to nullptr would be breaking something else.
I think it's also worth ensuring this works for Promises, requestAnimationFrame, etc. (I spoke to Dan in person).
(In reply to Geoffrey Garen from comment #22) > Comment on attachment 307623 [details] > 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. > Split off this renaming into bug #171145. > > 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. > Will fix. > > 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. > I refactored this code and removed the comment. > > 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. Will add a RAII object called BackupIncumbentScope to manage this. I chose to be more precise and call this class BackupIncumbentScope instead of IncumbentGlobalObjectScope because the scope is for the backup incumbent global object. The incumbent global object is defined to be the top-most author code global object falling back to the backup incumbent global object for scheduled author code.
Created attachment 309455 [details] Patch and layout tests Updated patch based on feedback from Geoffrey Garen and Saam Barati. With this patch applied we compute the incumbent global object correctly for all WebIDL callbacks and promise callbacks, including requestAnimationFrame(). I also added more tests to cover these cases.
Attachment 309455 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:35: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 5 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309455 [details] Patch and layout tests Attachment 309455 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3702628 Number of test failures exceeded the failure limit.
Created attachment 309458 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 309455 [details] Patch and layout tests Attachment 309455 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3702631 Number of test failures exceeded the failure limit.
Created attachment 309459 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 309455 [details] Patch and layout tests Attachment 309455 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3702639 Number of test failures exceeded the failure limit.
Created attachment 309460 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 309455 [details] Patch and layout tests Attachment 309455 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3702644 Number of test failures exceeded the failure limit.
Created attachment 309461 [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.11.6
Created attachment 309922 [details] Patch and layout tests
Attachment 309922 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/JSMutationCallback.cpp:46: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:35: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 6 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309922 [details] Patch and layout tests Attachment 309922 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3727746 New failing tests: fast/workers/worker-document-leak.html
Created attachment 309934 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 309922 [details] Patch and layout tests Attachment 309922 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3727772 New failing tests: fast/workers/worker-document-leak.html
Created attachment 309935 [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
Created attachment 309943 [details] Patch and layout tests Updated patch to have JSEventListener hold a weak pointer to the incumbent global object so as to fix the test failure fast/workers/worker-document-leak.html.
Attachment 309943 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/JSMutationCallback.cpp:46: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:35: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 6 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309943 [details] 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-execution-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.
(In reply to Geoffrey Garen from comment #44) > > 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. > Will do. > Let's rename this to callerGlobalObject. > Will do. > > 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-execution-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. Filed <https://github.com/whatwg/html/issues/2706>.
(In reply to Daniel Bates from comment #45) > > 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. > > Filed <https://github.com/whatwg/html/issues/2706>. In discussing <https://github.com/whatwg/html/issues/2706> I realized that the patch (attachment #309943 [details]) is incorrect because it stores the skip-when-determining-incumbent counter on the VM. But the skip-when-determining-incumbent counter is specific to each script-having execution context (ScriptExecutable?).
Created attachment 311315 [details] Patch and layout tests Updated patch that moves the skip-when-determining-incumbent counter to ScriptExecutable and addresses feedback from Geoffrey Garen. I also added test case "Invoke window B postMessage() from window A function called via DOM event dispatched to window A from event handler added by window B" to file LayoutTests/fast/dom/Window/post-message-source-srcdoc-iframe.html.
Created attachment 311319 [details] Patch and layout tests Rebase patch after <https://trac.webkit.org/changeset/217451> (bug #172601).
Attachment 311319 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/JSMutationCallback.cpp:46: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 6 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.
View in context: https://bugs.webkit.org/attachment.cgi?id=311315&action=review > Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:41 > + if (CodeBlock* callerCodeBlock = vm.topCallFrame ? vm.topCallFrame->callerCodeBlock() : nullptr) > + callerCodeBlock->ownerScriptExecutable()->incrementSkipWhenDeterminingIncumbentCounter(); Can you give some examples of when this object is instantiated and callerCodeBlock is not null? I can't seem to find any. It's also not clear to me why you would skip the top call frame's caller rather than the top call frame. You mentioned on IRC that all callbacks should now share the behavior that they propagate the caller that existed at the time the callback was scheduled. If that's the case, I don't understand why we need SkipWhenDeterminingIncumbentCounter.
(In reply to Geoffrey Garen from comment #50) > > Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:41 > > + if (CodeBlock* callerCodeBlock = vm.topCallFrame ? vm.topCallFrame->callerCodeBlock() : nullptr) > > + callerCodeBlock->ownerScriptExecutable()->incrementSkipWhenDeterminingIncumbentCounter(); > > Can you give some examples of when this object is instantiated and > callerCodeBlock is not null? I can't seem to find any. The following test cases, included in LayoutTests/fast/dom/Window/post-message-source-srcdoc-iframe.html, are examples when a BackupIncumbentScope is instantiated and CallFrame::callerCodeBlock() is non null: 1. "Invoke window A postMessage(), bound in window A, via DOM event dispatched to window A" This test case is analogous to example 3 in section Incumbent in the HTML standard <https://html.spec.whatwg.org/multipage/webappapis.html#incumbent> (24 May 2017). 2. "Invoke window B postMessage() from window A function called via DOM event dispatched to window A from event handler added by window B" > > It's also not clear to me why you would skip the top call frame's caller > rather than the top call frame. > Can you please elaborate? > You mentioned on IRC that all callbacks should now share the behavior that > they propagate the caller that existed at the time the callback was > scheduled. If that's the case, I don't understand why we need > SkipWhenDeterminingIncumbentCounter. From my understanding the existence of the skip-when-determining incumbent counter is solely to "fix up" the algorithm to work for event listeners :( See the above examples for more details.
Geoffrey Garen and I spoke about this bug in-person today (05/26). Geoffrey suggested that we implement the incumbent algorithm in parts to ensure correctness and allow us to identify ways to simplify the incumbent algorithm (if possible).
Any plans to fix this issue?
(In reply to Marian Rusnak from comment #53) > Any plans to fix this issue? Yes :) It is on my todo list. Having said that, feel free to take the bug and fix it!
Created attachment 443209 [details] WIP
Created attachment 443865 [details] WIP
Created attachment 444074 [details] Patch
Comment on attachment 444074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444074&action=review > Source/JavaScriptCore/interpreter/CallFrame.cpp:-252 > - // FIXME: We need to handle JSONP interpretation case in ProgramExecutable since it does not have vm.topCallFrame. > - // rdar://83691438 For this detail, I think we could consider just making a dummy call frame on the C++ stack before we do the JSONP execution.
Created attachment 449492 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
(In reply to Geoffrey Garen from comment #58) > Comment on attachment 444074 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=444074&action=review > > > Source/JavaScriptCore/interpreter/CallFrame.cpp:-252 > > - // FIXME: We need to handle JSONP interpretation case in ProgramExecutable since it does not have vm.topCallFrame. > > - // rdar://83691438 > > For this detail, I think we could consider just making a dummy call frame on > the C++ stack before we do the JSONP execution. I've added vm.JSONPGlobalObject field instead; making a dummy call frame that works across all tiers is a bit non-trivial.
Comment on attachment 449492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449492&action=review r=me Seems like there's work to make EWS happy before landing. > Source/JavaScriptCore/interpreter/Interpreter.cpp:757 > + SetForScope<JSGlobalObject*> changeJSONPGlobalObject(vm.JSONPGlobalObject, globalObject); Neat simplification. > Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:58 > + internalField(Field::IncumbentGlobalObject).setWithoutWriteBarrier(&JSC::incumbentGlobalObject(globalObject, vm.topCallFrame)); Makes me sad, but I guess we have to do it to match the spec. > Source/JavaScriptCore/runtime/RunCallbackScope.cpp:52 > + if (isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == TriState::True) > + return m_incumbentGlobalObject; I wonder if we should add an ASSERT_WITH_SECURITY_IMPLICATION here. We don't expect these to be different. Something like: auto isSameOriginDomain = topCallFrameGlobalObject->globalObjectMethodTable()->isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == TriState::True; ASSERT_WITH_SECURITY_IMPLICATION(isSameOriginDomain); if (isSameOriginDomain) return m_incumbentGlobalObject; Anyway, I like this compromise. It seems to ensure that the disappointing complexity of the incumbent global object spec can only create compatibility issues, and not security issues. > Source/JavaScriptCore/runtime/RunCallbackScope.h:33 > +class JS_EXPORT_PRIVATE RunCallbackScope final { I wonder if we should name this "AuthorCallbackScope" to clarify that you should deploy it any time you call into code that the spec would label "author" code. Or some other word to indicate "I'm calling out to author-specific JavaScript". I'm not sure that "Run" adds information, so "CallbackScope" might also be OK.
Created attachment 450276 [details] Patch Add missing null check, add ASSERT(isSameOriginDomain), rename to AuthorCallbackScope, and patch up significant Promise jobs regression.
Created attachment 450277 [details] Patch svn add <missing files>
Created attachment 450281 [details] Microbenchmark (In reply to Geoffrey Garen from comment #62) > r=me > > Seems like there's work to make EWS happy before landing. Thanks Geoff! EWS was failing due to missing null check for m_incumbentGlobalObject. What took some effort was patching up the significant performance regression of Promises. The previous patch regressed attached microbenchmark by ~35%: from 5.5s on ToT to around 7.4s, tested via `time Tools/Scripts/run-jsc --release microbenchmark.js` (first run after build to be ignored). This wasn't caught earlier because Tools/Scripts/run-jsc-benchmarks doesn't measure microtask loop execution. The 35% regression DOES NOT even include execution of isSameOriginDomain() as it's missing in JSC console, so the first thing I did is introduced incumbentGlobalObjectToStore(), which skips that check and is called only if resolved _incumbent_ isn't userland-exposed (happens very often). The second optimization was to introduce JSPromiseMicrotask for microtasks with userland code only, because adding Strong<> fields to JSMicrotask is costly. And the last thing was to avoid call stack walk during AuthorCallbackScope and defer it until _incumbent_ resolution, saving a m_topCallFrame pointer only. So the updated patch is only 7-8% slower in JSC console, which is IMO land-able, yet could be further improved by hand-rolling a faster version of CallFrame::globalObjectOfClosestCodeBlock(). I have a draft for that, it's a further 3-4% progression, yet the implementation that accounts for inlined DFG call frames is untrivial and it currently crashes. > > Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:58 > > + internalField(Field::IncumbentGlobalObject).setWithoutWriteBarrier(&JSC::incumbentGlobalObject(globalObject, vm.topCallFrame)); > > Makes me sad, but I guess we have to do it to match the spec. Yeah, the spec explicitly requires it here: https://html.spec.whatwg.org/#hostenqueuefinalizationregistrycleanupjob. This patch adds quite a few fields, yet we ensured the most of them are neutral on memory, unlike this one. However, JSFinalizationRegistry is a very new and very low-level addition to JS, so I may presume it won't be used much on the web? > > Source/JavaScriptCore/runtime/RunCallbackScope.cpp:52 > > + if (isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == TriState::True) > > + return m_incumbentGlobalObject; > > I wonder if we should add an ASSERT_WITH_SECURITY_IMPLICATION here. We don't > expect these to be different. Something like: > > auto isSameOriginDomain = > topCallFrameGlobalObject->globalObjectMethodTable()- > >isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == > TriState::True; > ASSERT_WITH_SECURITY_IMPLICATION(isSameOriginDomain); > if (isSameOriginDomain) > return m_incumbentGlobalObject; The linter prohibits ASSERT_WITH_SECURITY_IMPLICATION now, so I went with just ASSERT to ensure we don't crash. > > Source/JavaScriptCore/runtime/RunCallbackScope.h:33 > > +class JS_EXPORT_PRIVATE RunCallbackScope final { > > I wonder if we should name this "AuthorCallbackScope" to clarify that you > should deploy it any time you call into code that the spec would label > "author" code. Or some other word to indicate "I'm calling out to > author-specific JavaScript". That's a great suggestion, "author" is heavily used in the HTML spec. This naming makes very clear why, for example, JSMicrotask doesn't have AuthorCallbackScope: its job is never a userland code.
Using Example test case, I get following: ** Safari Technology Preview 152 ** Got message from A: hello ** Chrome Canary 107 ** Got message from B: hello ** Firefox Nightly 106 ** Got message from B: hello ___________ STP 152 differs from other browsers. Just want to update, I know it is 2022 bug but just want to highlight difference. Thanks!