Bug 188696

Summary: beforeunload interoperability issues
Product: WebKit Reporter: PhistucK <phistuck>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: achristensen, cdumez, darin, dbates, ews-watchlist, japhet, mkwst, rniwa
Priority: P2    
Version: Safari 11   
Hardware: Mac   
OS: macOS 10.13   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews114 for mac-sierra
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews115 for mac-sierra
none
Archive of layout-test-results from ews206 for win-future
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews200 for win-future
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews112 for mac-sierra none

Description PhistucK 2018-08-17 04:35:30 PDT
Assuming prior user interaction, the following code prevents a navigation, but should not -
window.onbeforeunload =
 function (e)
 {
  return {toString: function () { throw 'e' }};
 }
Also, it makes Safari briefly show an error information bar.
 
You can test it manually with -
https://plnkr.co/edit/PVx2JKzf0OUrOfZZ4BcR?p=preview
Click on the green full screen icon on the top right corner and click on the various cases and then click on "back" to attempt to trigger the beforeunload confirmation.

There is some disagreement among browsers around the edge cases.
The following ways prevent navigation (range indicates supported until) -
Way                             Chrome  Firefox Safari  Internet Explorer Edge
return                          15      3       4       6                 15
returnValue                     30      3       6.1     6                 15
preventDefault                  No      3       11      9                 15
return {toString: throw}        15 - 24 3 - 18  4*      No                No
returnValue = {toString: throw} No      No      No      9                 15

*Safari 5.1 - browser crash, Safari 10.1 and 11.1 - looks like an automatically recovering tab crash.

Note -
The tests were run using BrowserStack, so the lowest versions available are -
Firefox 3
Safari 4
Internet Explorer 6
Edge 15.
Support in prior versions cannot be determined.

Note that Chrome used to support a throwing return, but stopped. Firefox, too.
Only Microsoft browsers support a throwing returnValue.
Only Safari supports a throwing return.

My interpretation of the specification is the same for return and returnValue. The value must be converted to a DOMString when set/returned. Since it cannot be converted, the value is the initial value/undefined/null/empty string. Null and empty strings are not supposed to prevent the navigation.
I think only Firefox is currently compliant with the specification.

https://html.spec.whatwg.org/multipage/webappapis.html#onbeforeunloadeventhandlernonnull
https://html.spec.whatwg.org/multipage/browsing-the-web.html#dom-beforeunloadevent-returnvalue
https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm - "If an exception gets thrown by the callback, end these steps and allow the exception to propagate" (the next step cancels the event, so that does not happen)
https://html.spec.whatwg.org/multipage/browsing-the-web.html#prompt-to-unload-a-document


Original Chromium code review discussion -
https://chromium-review.googlesource.com/c/chromium/src/+/1154225/9..10#message-8751c40094a6e40fe00991099d5d3e71cf9c076e
Comment 1 PhistucK 2018-08-17 06:48:58 PDT
I think the problem is either that convert<IDLNullable<IDLDOMString>>(*exec, retval)) is called but nothing checks whether it threw an exception in order to stop the execution, or that the conversion that convert<...> employs does not use toStringOrNull and so it gets the empty string instead of null when it fails.
handleBeforeUnloadEventReturnValue only checks -
if (returnValue.isNull())
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/bindings/js/JSEventListener.cpp#L91
And preventDefault() otherwise.

Or maybe both.

Maybe the fix is a simple -
if (returnValue.isNull() || returnValue.isEmpty())
Comment 2 PhistucK 2018-08-17 14:27:59 PDT
Created attachment 347390 [details]
Patch
Comment 3 PhistucK 2018-08-17 14:49:44 PDT
I am sorry in advance, I have a Linux machine and I tried to build WebKit(GTK), but it failed at a fairly early stage (jhbuild dependencies, probably due to a much too new Clang that breaks Pixman), so this patch is untested.
Anyone is welcome to take it (no credit/attribution is needed) and submit it as their own.
Comment 4 EWS Watchlist 2018-08-17 15:19:29 PDT
Comment on attachment 347390 [details]
Patch

Attachment 347390 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8895158

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
Comment 5 EWS Watchlist 2018-08-17 15:19:31 PDT
Created attachment 347395 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 EWS Watchlist 2018-08-17 15:30:40 PDT
Comment on attachment 347390 [details]
Patch

Attachment 347390 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8895178

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
Comment 7 EWS Watchlist 2018-08-17 15:30:42 PDT
Created attachment 347397 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 8 EWS Watchlist 2018-08-17 15:59:01 PDT
Comment on attachment 347390 [details]
Patch

Attachment 347390 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8895270

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
Comment 9 EWS Watchlist 2018-08-17 15:59:03 PDT
Created attachment 347403 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2018-08-17 16:23:58 PDT
Comment on attachment 347390 [details]
Patch

Attachment 347390 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8895365

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
Comment 11 EWS Watchlist 2018-08-17 16:24:00 PDT
Created attachment 347410 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 12 PhistucK 2018-08-17 23:29:44 PDT
This is the apparent failure -
FAIL Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable assert_false: The event must not have been canceled expected false got true

Huh, why would it affect a CustomEvent of type "beforeunload"?
This code path is only reached if is<BeforeUnloadEvent>(event)...
Comment 13 PhistucK 2018-08-18 00:54:19 PDT
Oops, I looked at the wrong failure. Investigating.
Comment 14 PhistucK 2018-08-18 04:26:26 PDT
Created attachment 347442 [details]
Patch
Comment 15 Darin Adler 2018-08-19 17:15:19 PDT
Comment on attachment 347442 [details]
Patch

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

Thanks for contributing this patch. Seems like a good change to make! Almost there, but needs a little work. Need to add a test, and there’s a mistake in the patch that a good test would find.

> Source/WebCore/ChangeLog:8
> +        No new tests .

Normally we’d say *why* there are no new tests. If there is a change in behavior, the project requires we add a test to demonstrate that the patch works. Can we construct a test? I think we should be able to do this by passing an object that shows an exception in its valueOf function. We should look at our existing beforeunload tests to see if one can be adapted for this purpose. Ideally another test will cover the case where the valueOf function does not return an exception and ensure that it is called exactly once. That would have caught the mistake in the patch that I mention below.

> Source/WebCore/bindings/js/JSEventListener.cpp:194
> +                    reportException(exec, exception);

We might need something like the call to uncaughtExceptionInEventHandler above. Not sure if we need it. Might require a little research.

> Source/WebCore/bindings/js/JSEventListener.cpp:196
> +                    handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event), convert<IDLNullable<IDLDOMString>>(*exec, retval));

This needs to pass returnedString, and not compute it a second time.
Comment 16 Darin Adler 2018-08-19 17:15:55 PDT
I said valueOf, but toString I guess is what I should have said.
Comment 17 PhistucK 2018-08-19 22:57:23 PDT
(In reply to Darin Adler from comment #15)
Thank you for the review and sorry for wasting your time.
I agree with you that the patch is nearly ready for being committed.
The problem is that I do not have a suitable development environment (no macOS and my remote Linux - powered by Janitor - has Clang 5+, so WebKitGTK fails to build its dependencies), so I cannot either compile, manually test my code, or run the automated tests.
(See comment 3)


> Comment on attachment 347442 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347442&action=review
> 
> Thanks for contributing this patch. Seems like a good change to make! Almost
> there, but needs a little work. Need to add a test, and there’s a mistake in
> the patch that a good test would find.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests .
> 
> Normally we’d say *why* there are no new tests.

I was too embarrassed to write, "Because, hell, I did not even test it manually". :P

> If there is a change in
> behavior, the project requires we add a test to demonstrate that the patch
> works. Can we construct a test? I think we should be able to do this by
> passing an object that shows an exception in its valueOf function. We should
> look at our existing beforeunload tests to see if one can be adapted for
> this purpose.

Yeah, makes sense, I felt more comfortable blindingly writing code than blindingly writing a test. :P

> Ideally another test will cover the case where the valueOf
> function does not return an exception and ensure that it is called exactly
> once.

Huh, would you have added/thought about adding that test if I did not write that mistake? Is there such a test for every case that implicitly/explicitly calls toString? If not, do you think there should be one for every case? It seems to me that this would inflate the test suite considerably (and maybe also should be handled at the compiled IDL binding level instead somehow).
I do not mind having such a test (of course it would have caught my mistake), I just wonder if you think such practice should be extremely widespread.
(I do understand my mistake exists/existed in the code base for other cases that you had to fix, which is probably what prompts the request for such a test)

> That would have caught the mistake in the patch that I mention below.
> 
> > Source/WebCore/bindings/js/JSEventListener.cpp:194
> > +                    reportException(exec, exception);
> 
> We might need something like the call to uncaughtExceptionInEventHandler
> above. Not sure if we need it. Might require a little research.

Without being able to compile/test, a research would be hard to commence... I might try to consult with some GTK folks about overcoming my Clang 5+ problem without downgrading Clang.

> 
> > Source/WebCore/bindings/js/JSEventListener.cpp:196
> > +                    handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event), convert<IDLNullable<IDLDOMString>>(*exec, retval));
> 
> This needs to pass returnedString, and not compute it a second time.

Correct, I will fix this.
Comment 18 Ryosuke Niwa 2018-08-20 12:21:22 PDT
(In reply to PhistucK from comment #17)
>
> > Comment on attachment 347442 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=347442&action=review
> > 
> > Thanks for contributing this patch. Seems like a good change to make! Almost
> > there, but needs a little work. Need to add a test, and there’s a mistake in
> > the patch that a good test would find.
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        No new tests .
> > 
> > Normally we’d say *why* there are no new tests.
> 
> I was too embarrassed to write, "Because, hell, I did not even test it
> manually". :P

We definitely need a layout test for this code change.

> > Ideally another test will cover the case where the valueOf
> > function does not return an exception and ensure that it is called exactly
> > once.
> 
> Huh, would you have added/thought about adding that test if I did not write
> that mistake?

The fact our existing test coverage didn't catch the bug Darin pointed out is a good evidence that we need more tests in this area. In general, it's a good practice to add more test coverage whenever someone makes a coding mistake, and that coding mistake wasn't caught by an existing test case.

This is why, for example, when I fix a bug or implement a new feature, I'd purposefully not implement the full fix, and see if any existing tests or imported tests would fail. Ideally, we would be testing every line of code we write one way or another. In some cases that's not practical due to timing etc... but we should strive to do so as much as we can.
Comment 19 Darin Adler 2018-08-24 14:54:41 PDT
(In reply to PhistucK from comment #17)
> Huh, would you have added/thought about adding that test if I did not write
> that mistake?

Yes. I am deeply invested in creating more tests about JavaScript side effects and exceptions. This is an area we often don’t cover sufficiently in our testing and is easy to get wrong. Such mistakes can create interoperability problems between web browsers and security vulnerabilities. But I also find it inelegant and aesthetically repugnant to have side effects that occur in a unpredictable order or code that runs after an exception has been raised.

> Is there such a test for every case that implicitly/explicitly
> calls toString?

No.

> If not, do you think there should be one for every case?

I do.

> It seems to me that this would inflate the test suite considerably

This does not worry me.

> (and maybe
> also should be handled at the compiled IDL binding level instead somehow).

If you mean that the code to handle issues like this one should be generated from IDL whenever possible, I agree. It’s trickier to get this right in cases that are hand-written rather than generated. But of course we are having this discussion in the context of a case where this is hand-written code, and is not generated.

If the code was generated we might decide we don’t need individual test coverage for all the generated cases. Or maybe not, it’s possible we can cover all the cases efficiently and so it’s worth doing do. Typically it’s not a challenging trade-off. With a few exceptions, more tests usually do work out to be affordable.

If you mean that the code to *test* these cases should be generated from IDL whenever possible, yes, I agree that is a good strategy too. But again it’s not a reason not to write a single test when we aren’t yet generating such things.

> I do not mind having such a test (of course it would have caught my
> mistake), I just wonder if you think such practice should be extremely
> widespread.

I do.

> (I do understand my mistake exists/existed in the code base for other cases
> that you had to fix, which is probably what prompts the request for such a
> test)

Exactly.

Our strategy in this project is to test modified code which we had to visit to fix a bug or add a feature even *more* thoroughly than the rest of the code in the project; in many cases doing this then leads, either directly or indirectly, to thoroughly testing the similar issue across many or all cases.
Comment 20 PhistucK 2018-08-24 15:19:24 PDT
@Darin and @Ryosuke - I agree that more tests are better than less tests.
I guess my fear of inflating the test suite comes from a thread regarding a totally different discussion I read some time ago that (from my sifting) regarded this as a serious problem -
https://lists.webkit.org/pipermail/webkit-dev/2017-November/029752.html

(It was actually quite traumatic to read on that day, but perhaps I simply misunderstood...)

Next steps -
- Fix the double evaluation issue.
- Research uncaughtExceptionInEventHandler.
- Add/adapt with required tests -
 - return {toString() { throw 'e' }} does not prevent navigation.
 - return {toString() { ++foo; throw 'e' }} as well as return {toString() { ++foo; return 'e'}} does not increase foo to more than 1.

Let me know if I missed anything, or if anything else is needed.
Comment 21 PhistucK 2018-09-22 01:42:10 PDT
Created attachment 350504 [details]
Patch
Comment 22 PhistucK 2018-09-22 01:54:40 PDT
So, yes, a big patch, but mostly because of extensive test cases.
I basically covered everything in https://next.plnkr.co/edit/OpgW2EvPSh0JbkLwXDU6?p=preview&utm_source=legacy&utm_medium=worker&utm_campaign=next&preview
And then some, including within iFrames.
I might have gone a bit overboard with this, however, I think the tests now cover every tiny bit of the specification.
(It is a shame those cannot be web-platform-tests :( due to the user activation requirement)

uncaughtExceptionInEventHandler is used by IndexedDB, for rolling back a transaction. Even though beforeunload is currently unused/not fired by IndexedDB, I added it, for completeness.

In addition to the throwing return issue, I added a new fix for the following case -
addEventListener("beforeunload", () => "string")
Per the DOM standard, the return value of the listener in addEventListener is completely ignored, but only WebKit does not ignore it and only for beforeunload. This looks like an accident rather than an intentional design/feature. Since no other browser supports this questionable feature (even Edge that wants to be compatible with WebKit/Blink) at this point, I fixed it to be ignored, like with the rest of the events.

I had to fix a test as a result of this fix, but the original test case in https://bugs.webkit.org/show_bug.cgi?id=26211 did not use addEventListener and returned a string, it used onbeforeunload and returned a string, so I assume it was just an accident in the test writing (changing to addEventListener and returning a string still worked, so there was nothing to fix/change).
Comment 23 EWS Watchlist 2018-09-22 02:26:13 PDT
Comment on attachment 350504 [details]
Patch

Attachment 350504 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9309747

Number of test failures exceeded the failure limit.
Comment 24 EWS Watchlist 2018-09-22 02:26:15 PDT
Created attachment 350510 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 25 EWS Watchlist 2018-09-22 02:50:23 PDT
Comment on attachment 350504 [details]
Patch

Attachment 350504 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9309758

Number of test failures exceeded the failure limit.
Comment 26 EWS Watchlist 2018-09-22 02:50:24 PDT
Created attachment 350511 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 27 EWS Watchlist 2018-09-22 03:10:50 PDT
Comment on attachment 350504 [details]
Patch

Attachment 350504 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9310016

Number of test failures exceeded the failure limit.
Comment 28 EWS Watchlist 2018-09-22 03:10:56 PDT
Created attachment 350513 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 29 EWS Watchlist 2018-09-22 03:12:36 PDT
Comment on attachment 350504 [details]
Patch

Attachment 350504 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9309860

Number of test failures exceeded the failure limit.
Comment 30 EWS Watchlist 2018-09-22 03:12:38 PDT
Created attachment 350514 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 31 Ryosuke Niwa 2018-09-22 03:12:51 PDT
Comment on attachment 350504 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

The change log should state why we're making this change, reference relevant spec text, and explain how we're implementing that.
r- due to the lack of the change log description.

> LayoutTests/ChangeLog:9
> +        Had to remove the event listeners once the test is done, otherwise
> +        they would show a beforeunload alert on the next test for some reason.

We should investigate why that's happening and fix WebCore / WebKitTestRunner / DumpRenderTree instead of updating tests.
r- because of this.
Comment 32 PhistucK 2018-09-22 04:54:28 PDT
The reason looks clear, though - the test runner is probably navigating to the next test...
Comment 33 PhistucK 2018-09-22 07:10:27 PDT
@Ryosuke - if you think this must be fixed before this patch is accepted, I will really need pointers here...
I tried to trace the code (pretty manually, though) and could not quite figure out how the test runner is running a test.

I searched using GitHub (not great, I know, but it is faster than grepping locally) and this is as far as I got -
https://github.com/WebKit/webkit/search?p=2&q=driver_name&unscoped_q=driver_name
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Tools/Scripts/webkitpy/port/xorgdriver.py
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Tools/Scripts/webkitpy/port/xorgdriver.py
https://github.com/WebKit/webkit/search?utf8=%E2%9C%93&q=WebKitTestRunner&type=
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Tools/WebKitTestRunner/TestController.cpp
https://github.com/WebKit/webkit/tree/89c28d471fae35f1788a0f857067896a10af8974/Tools/WebKitTestRunner
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Tools/WebKitTestRunner/TestController.cpp
https://github.com/WebKit/webkit/search?q=platformrunUntil&unscoped_q=platformrunUntil
https://github.com/WebKit/webkit/blob/a6ea03d7ff7d26087b291e2ce5e9857d5903fb0f/Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Tools/WebKitTestRunner/TestInvocation.cpp
https://github.com/WebKit/webkit/search?utf8=%E2%9C%93&q=WKPagePostMessageToInjectedBundle&type=
https://github.com/WebKit/webkit/blob/master/Source/WebKit/UIProcess/API/C/WKPage.cpp
https://github.com/WebKit/webkit/search?utf8=%E2%9C%93&q=postMessageToInjectedBundle&type=
https://github.com/WebKit/webkit/blob/2df33d3721be95634af5852a635fee3711db5c4a/Source/WebKit/UIProcess/WebProcessPool.cpp
https://github.com/WebKit/webkit/blob/9029c43e695bf886fffb15eec951f0605e34509b/Source/WebKit/UIProcess/WebPageProxy.cpp
https://github.com/WebKit/webkit/search?utf8=%E2%9C%93&q=PostInjectedBundleMessage&type=
https://github.com/WebKit/webkit/blob/ce279990f85656893c57decbffe2b9067176d02e/Source/WebKit/WebProcess/WebPage/WebPage.cpp
https://github.com/WebKit/webkit/search?p=2&q=didReceiveMessageToPage&type=&utf8=%E2%9C%93
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Source/WebKit/WebProcess/InjectedBundle/InjectedBundleClient.h
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Tools/TestWebKitAPI/InjectedBundleController.cpp#L111
https://github.com/WebKit/webkit/blob/1b5109a8ae12c0484f3104c5c7b3c181b44ebb06/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
https://github.com/WebKit/webkit/search?utf8=%E2%9C%93&q=WKBundlePagePostMessage&type=
https://github.com/WebKit/webkit/blob/1b5109a8ae12c0484f3104c5c7b3c181b44ebb06/Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp
https://github.com/WebKit/webkit/search?l=C%2B%2B&p=3&q=postMessage+Page
https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp
https://github.com/WebKit/webkit/blob/ce279990f85656893c57decbffe2b9067176d02e/Source/WebKit/WebProcess/WebPage/WebPage.cpp
https://github.com/WebKit/webkit/search?l=C%2B%2B&p=2&q=HandleMessage&type=
https://github.com/WebKit/webkit/blob/9029c43e695bf886fffb15eec951f0605e34509b/Source/WebKit/UIProcess/WebPageProxy.cpp
https://github.com/WebKit/webkit/search?p=2&q=didReceiveMessageFromInjectedBundle&type=&utf8=%E2%9C%93



I must have missed something, but I figure you can save me days of investigating this.
Perhaps this is just a matter of emptying the beforeunload listeners before starting a test? Or maybe ignoring previous test output before the test has started loading/parsing its case?
Either way, I am not sure where to begin.

I guess the resetting should run in the InjectedBundle::beginTesting method -
https://github.com/WebKit/webkit/blob/1b5109a8ae12c0484f3104c5c7b3c181b44ebb06/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp#L460

I am not sure what is the correct approach. Please, advise.


Looks like this is getting way bigger than I anticipated. :(
Comment 34 PhistucK 2018-09-22 08:43:29 PDT
Created attachment 350519 [details]
Patch
Comment 35 PhistucK 2018-09-22 08:45:33 PDT
I amended the change log, added expectations for GTK and fixed the base expectations.

There seems to be some threading issue going on, even in non-GTK, because in some tests, console.log of return {toString} shows up after the beforeunload alert. Any idea about that?
Comment 36 EWS Watchlist 2018-09-22 09:39:26 PDT
Comment on attachment 350519 [details]
Patch

Attachment 350519 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9312317

Number of test failures exceeded the failure limit.
Comment 37 EWS Watchlist 2018-09-22 09:39:28 PDT
Created attachment 350520 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 38 EWS Watchlist 2018-09-22 09:50:20 PDT
Comment on attachment 350519 [details]
Patch

Attachment 350519 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9312358

New failing tests:
fast/events/beforeunload/onbeforeunload-returnValue-handler-exception.html
fast/events/beforeunload/onbeforeunload-return-and-returnValue.html
fast/events/beforeunload/onbeforeunload-return.html
fast/events/beforeunload/onbeforeunload-returnValue.html
Comment 39 EWS Watchlist 2018-09-22 09:50:22 PDT
Created attachment 350521 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 40 EWS Watchlist 2018-09-22 10:14:33 PDT
Comment on attachment 350519 [details]
Patch

Attachment 350519 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9312355

Number of test failures exceeded the failure limit.
Comment 41 EWS Watchlist 2018-09-22 10:14:35 PDT
Created attachment 350522 [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.13.6
Comment 42 EWS Watchlist 2018-09-22 10:18:00 PDT
Comment on attachment 350519 [details]
Patch

Attachment 350519 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9312449

New failing tests:
fast/events/beforeunload/onbeforeunload-returnValue-handler-exception.html
fast/events/beforeunload/onbeforeunload-return-and-returnValue.html
fast/events/beforeunload/onbeforeunload-return.html
fast/events/beforeunload/onbeforeunload-returnValue.html
Comment 43 EWS Watchlist 2018-09-22 10:18:12 PDT
Created attachment 350523 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 44 EWS Watchlist 2018-09-22 10:48:00 PDT
Comment on attachment 350519 [details]
Patch

Attachment 350519 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9312458

New failing tests:
fast/events/beforeunload/onbeforeunload-returnValue-handler-exception.html
fast/events/beforeunload/onbeforeunload-return-and-returnValue.html
fast/events/beforeunload/onbeforeunload-return.html
fast/events/beforeunload/onbeforeunload-returnValue.html
Comment 45 EWS Watchlist 2018-09-22 10:48:03 PDT
Created attachment 350524 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 46 PhistucK 2018-09-22 12:07:44 PDT
Right, so, the logging order is unstable between operating system versions/ports, or simply in general.
:(

So there are a few testing issues -
1. The order of console.log and beforeunload alert is unpredictable.
2. beforeunload alerts are also shown on the next test.
3. testRunner.dumpAsText() does not dump text of iFrames.
4. testRunner.dumpAsText() does not dump text across navigation (I am not completely sure about that one). 

The last three can be worked around (by removing listeners and by using console.log), but then the first one kicks in.
:(

It is a bit much for me to investigate, seeing as this is my first WebKit C++ patch (and my first C++ patch in general, I think, hehe) and seeing as I am more of a web developer and seeing as I do not have a local development machine (or a relatively fast remote development machine), or macOS.

I would appreciate assistance from experienced WebKit engineers.
Comment 47 EWS Watchlist 2018-09-22 12:43:30 PDT
Comment on attachment 350519 [details]
Patch

Attachment 350519 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9313024

New failing tests:
fast/events/beforeunload/onbeforeunload-returnValue-handler-exception.html
fast/events/beforeunload/onbeforeunload-return-and-returnValue.html
fast/events/beforeunload/onbeforeunload-return.html
fast/events/beforeunload/onbeforeunload-returnValue.html
Comment 48 EWS Watchlist 2018-09-22 12:43:32 PDT
Created attachment 350528 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 49 Alex Christensen 2021-11-01 12:56:22 PDT
Comment on attachment 350519 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.