Bug 168382 – onbeforeunload event return value coercion is not per-spec

• Component: Page Loading
• Status: RESOLVED FIXED

[Domenic Denicola]

Test: http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
Spec: https://html.spec.whatwg.org/#the-event-handler-processing-algorithm step 4

The return value of an onbeforeunload event handler is supposed to be coerced to a DOMString?, and then any non-null value should cancel the event. That means anything but null or undefined should cancel the event. Safari Tech Preview seems to not cancel when returning "" or true or 0.

[Chris Dumez]

Unfortunately, the test in question is flaky on WebKit. It sometimes starts playing with the frames before their content has loaded, thus throwing exceptions.

[Domenic Denicola]

Hmm, that shouldn't be possible, at least, I can't see how this code would cause it:

  async_test(t => {
    const iframe = document.createElement("iframe");
    iframe.onload = t.step_func(() => {
      iframe.contentWindow.runTest(t, testCase);
    });

    iframe.src = "beforeunload-canceling-1.html";
    document.body.appendChild(iframe);
  }, `Returning ${testCase.valueToReturn} with a real iframe unloading${labelAboutReturnValue}`);

[Chris Dumez]

(In reply to comment #2)
> Hmm, that shouldn't be possible, at least, I can't see how this code would
> cause it:
> 
>   async_test(t => {
>     const iframe = document.createElement("iframe");
>     iframe.onload = t.step_func(() => {
>       iframe.contentWindow.runTest(t, testCase);
>     });
> 
>     iframe.src = "beforeunload-canceling-1.html";
>     document.body.appendChild(iframe);
>   }, `Returning ${testCase.valueToReturn} with a real iframe
> unloading${labelAboutReturnValue}`);

frame.contentWindow.runTest is not a function. (In 'iframe.contentWindow.runTest(t, testCase)', 'iframe.contentWindow.runTest' is undefined)
http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html:128:35
step@http://w3c-test.org/resources/testharness.js:1409:30
http://w3c-test.org/resources/testharness.js:1433:40
appendChild@[native code]
http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html:132:30
step@http://w3c-test.org/resources/testharness.js:1409:30
async_test@http://w3c-test.org/resources/testharness.js:518:26
global code@http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html:125:13

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > Hmm, that shouldn't be possible, at least, I can't see how this code would
> > cause it:
> > 
> >   async_test(t => {
> >     const iframe = document.createElement("iframe");
> >     iframe.onload = t.step_func(() => {
> >       iframe.contentWindow.runTest(t, testCase);
> >     });
> > 
> >     iframe.src = "beforeunload-canceling-1.html";
> >     document.body.appendChild(iframe);
> >   }, `Returning ${testCase.valueToReturn} with a real iframe
> > unloading${labelAboutReturnValue}`);
> 
> frame.contentWindow.runTest is not a function. (In
> 'iframe.contentWindow.runTest(t, testCase)', 'iframe.contentWindow.runTest'
> is undefined)
> http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/
> beforeunload-canceling.html:128:35
> step@http://w3c-test.org/resources/testharness.js:1409:30
> http://w3c-test.org/resources/testharness.js:1433:40
> appendChild@[native code]
> http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/
> beforeunload-canceling.html:132:30
> step@http://w3c-test.org/resources/testharness.js:1409:30
> async_test@http://w3c-test.org/resources/testharness.js:518:26
> global
> code@http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/
> beforeunload-canceling.html:125:13

Maybe the unload handler sometimes gets called for the 'about:blank' load somehow? Or the page really has loaded but runTest has not been defined yet when the load event fires.

[Domenic Denicola]

Hmm, those make sense, especially the about:blank one. I unfortunately cannot reproduce on my Mac; would you mind working on a test fix, since you are able to reproduce?

[Chris Dumez]

Created attachment 301688 [details]
Patch

[Chris Dumez]

ping review? Let me know if you'd like me to split the patch. It is not big but it fixes several related bugs in the same patch.

[Daniel Bates]

Comment on attachment 301688 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:3039
> -    String text = document->displayStringModifiedByEncoding(beforeUnloadEvent->returnValue());
> -    return chrome.runBeforeUnloadConfirmPanel(text, m_frame);
> +    // The specification says "The message shown to the user is not customizable, but instead determined by the user agent.
> +    // In particular, the actual value of the returnValue attribute is ignored". Therefore, we ignore the returnValue here
> +    // and pass a null string.
> +    return chrome.runBeforeUnloadConfirmPanel(String(), m_frame);

This breaks public WebKit Legacy API on macOS: <https://developer.apple.com/reference/webkit/webuidelegate/1387844-webview?language=objc>. Is this acceptable?

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 301688 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301688&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:3039
> > -    String text = document->displayStringModifiedByEncoding(beforeUnloadEvent->returnValue());
> > -    return chrome.runBeforeUnloadConfirmPanel(text, m_frame);
> > +    // The specification says "The message shown to the user is not customizable, but instead determined by the user agent.
> > +    // In particular, the actual value of the returnValue attribute is ignored". Therefore, we ignore the returnValue here
> > +    // and pass a null string.
> > +    return chrome.runBeforeUnloadConfirmPanel(String(), m_frame);
> 
> This breaks public WebKit Legacy API on macOS:
> <https://developer.apple.com/reference/webkit/webuidelegate/1387844-
> webview?language=objc>. Is this acceptable?

That is a fair point.

Would you find it acceptable to pass the returnValue to the delegate but not print out its value in the layout tests (as this causes flakiness)?

[Daniel Bates]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 301688 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=301688&action=review
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:3039
> > > -    String text = document->displayStringModifiedByEncoding(beforeUnloadEvent->returnValue());
> > > -    return chrome.runBeforeUnloadConfirmPanel(text, m_frame);
> > > +    // The specification says "The message shown to the user is not customizable, but instead determined by the user agent.
> > > +    // In particular, the actual value of the returnValue attribute is ignored". Therefore, we ignore the returnValue here
> > > +    // and pass a null string.
> > > +    return chrome.runBeforeUnloadConfirmPanel(String(), m_frame);
> > 
> > This breaks public WebKit Legacy API on macOS:
> > <https://developer.apple.com/reference/webkit/webuidelegate/1387844-
> > webview?language=objc>. Is this acceptable?
> 
> That is a fair point.
> 
> Would you find it acceptable to pass the returnValue to the delegate but not
> print out its value in the layout tests (as this causes flakiness)?

It would be good to understand why we have flakiness? In particular, why does the order of the confirm delegate callbacks change? I mean, I would have assumed they are blocking calls to the UI delegate to ask for confirmation and inconsistent ordering could affect third-part apps.

Regardless, we need to ensure that we do not regress the behavior of the WebKit Legacy API so long as we support it.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 301688 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=301688&action=review
> > > 
> > > > Source/WebCore/loader/FrameLoader.cpp:3039
> > > > -    String text = document->displayStringModifiedByEncoding(beforeUnloadEvent->returnValue());
> > > > -    return chrome.runBeforeUnloadConfirmPanel(text, m_frame);
> > > > +    // The specification says "The message shown to the user is not customizable, but instead determined by the user agent.
> > > > +    // In particular, the actual value of the returnValue attribute is ignored". Therefore, we ignore the returnValue here
> > > > +    // and pass a null string.
> > > > +    return chrome.runBeforeUnloadConfirmPanel(String(), m_frame);
> > > 
> > > This breaks public WebKit Legacy API on macOS:
> > > <https://developer.apple.com/reference/webkit/webuidelegate/1387844-
> > > webview?language=objc>. Is this acceptable?
> > 
> > That is a fair point.
> > 
> > Would you find it acceptable to pass the returnValue to the delegate but not
> > print out its value in the layout tests (as this causes flakiness)?
> 
> It would be good to understand why we have flakiness? In particular, why
> does the order of the confirm delegate callbacks change? I mean, I would
> have assumed they are blocking calls to the UI delegate to ask for
> confirmation and inconsistent ordering could affect third-part apps.

The web-platform-test uses a lot of frames and relies on their load event firing to run each subtest. The order of the load events is not pre-determined so it makes sense that the CONFIRM messages order can change.

> 
> Regardless, we need to ensure that we do not regress the behavior of the
> WebKit Legacy API so long as we support it.

Agreed, I will keep passing the string.

[Chris Dumez]

Created attachment 301970 [details]
Patch

[Chris Dumez]

Comment on attachment 301970 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt:1
> +CONFIRM NAVIGATION

If people feel strongly about printing the returnValue. I can also propose a change to the web-platform-test upstream to make the order deterministic.
I personally do not feel those messages bring much value here and the specification says to ignore the message, which is why I went this way.

[Daniel Bates]

(In reply to comment #11)
> > 
> > Regardless, we need to ensure that we do not regress the behavior of the
> > WebKit Legacy API so long as we support it.
> 
> Agreed, I will keep passing the string.

When I wrote this comment I was implying that we need a way to ensure that we do not regress the behavior of the WebKit Legacy API webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame:. We should not remove the existing test infrastructure that we have (printing the confirm message in layout tests) that ensures that webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame: is called and is passed the confirmation message without having some replacement testing methodology to catch such regressions in behavior. I can think of at least two options: 1. Replace the absence of DumpRenderTree/WebKitTestRunner emitting the confirmation message with TestWebKitAPI test(s) or 2. Provide a window.testRunner setting to disable output of the confirmation message in DumpRenderTree/WebKitTestRunner and enable this setting in web-platform-tests or other tests as needed. I slightly prefer the latter option as it sounds the flakiness is specific to the web-plaoform-tests based on comment 11.

[Daniel Bates]

(In reply to comment #13)
> Comment on attachment 301970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301970&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt:1
> > +CONFIRM NAVIGATION
> 
> [...]
> I can also propose a
> change to the web-platform-test upstream to make the order deterministic.

Please propose such a change. I do not see any benefit to having such non-determinism. It makes reasoning about test failure difficult.

[Daniel Bates]

Comment on attachment 301970 [details]
Patch

We should not remove exiting testing support for webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame: without proposing an alternative way of ensuring we do not regress it.

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #11)
> > > 
> > > Regardless, we need to ensure that we do not regress the behavior of the
> > > WebKit Legacy API so long as we support it.
> > 
> > Agreed, I will keep passing the string.
> 
> When I wrote this comment I was implying that we need a way to ensure that
> we do not regress the behavior of the WebKit Legacy API
> webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame:. We should
> not remove the existing test infrastructure that we have (printing the
> confirm message in layout tests) that ensures that
> webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame: is called
> and is passed the confirmation message without having some replacement
> testing methodology to catch such regressions in behavior. I can think of at
> least two options: 1. Replace the absence of DumpRenderTree/WebKitTestRunner
> emitting the confirmation message with TestWebKitAPI test(s) or 2. Provide a
> window.testRunner setting to disable output of the confirmation message in
> DumpRenderTree/WebKitTestRunner and enable this setting in
> web-platform-tests or other tests as needed. I slightly prefer the latter
> option as it sounds the flakiness is specific to the web-plaoform-tests
> based on comment 11.

Ok, I'll do a PR to fix the web-platform-test.

[Chris Dumez]

(In reply to comment #16)
> Comment on attachment 301970 [details]
> Patch
> 
> We should not remove exiting testing support for
> webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame: without
> proposing an alternative way of ensuring we do not regress it.

(In reply to comment #15)
> (In reply to comment #13)
> > Comment on attachment 301970 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=301970&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt:1
> > > +CONFIRM NAVIGATION
> > 
> > [...]
> > I can also propose a
> > change to the web-platform-test upstream to make the order deterministic.
> 
> Please propose such a change. I do not see any benefit to having such
> non-determinism. It makes reasoning about test failure difficult.

The benefit is that the test likely runs faster but hopefully they will accept the change upstream.

[Chris Dumez]

Created attachment 301977 [details]
Patch

[Chris Dumez]

Created attachment 301979 [details]
Patch

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #11)
> > > > 
> > > > Regardless, we need to ensure that we do not regress the behavior of the
> > > > WebKit Legacy API so long as we support it.
> > > 
> > > Agreed, I will keep passing the string.
> > 
> > When I wrote this comment I was implying that we need a way to ensure that
> > we do not regress the behavior of the WebKit Legacy API
> > webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame:. We should
> > not remove the existing test infrastructure that we have (printing the
> > confirm message in layout tests) that ensures that
> > webView:runBeforeUnloadConfirmPanelWithMessage:initiatedByFrame: is called
> > and is passed the confirmation message without having some replacement
> > testing methodology to catch such regressions in behavior. I can think of at
> > least two options: 1. Replace the absence of DumpRenderTree/WebKitTestRunner
> > emitting the confirmation message with TestWebKitAPI test(s) or 2. Provide a
> > window.testRunner setting to disable output of the confirmation message in
> > DumpRenderTree/WebKitTestRunner and enable this setting in
> > web-platform-tests or other tests as needed. I slightly prefer the latter
> > option as it sounds the flakiness is specific to the web-plaoform-tests
> > based on comment 11.
> 
> Ok, I'll do a PR to fix the web-platform-test.

Done: https://github.com/w3c/web-platform-tests/pull/4920

[Darin Adler]

Comment on attachment 301979 [details]
Patch

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

Looks great. Some thoughts on how to be even more thorough.

> Source/WebCore/ChangeLog:15
> +          explicitely been set by JS).

Typo here in the word explicitly.

> Source/WebCore/bindings/js/JSEventListener.cpp:166
> +                    beforeUnloadEvent.setReturnValue(retval.toWTFString(exec));

I believe that conversion to string can have an observable side effect, due to the fact that toPrimitive gets called on the value if it’s an object. Would be super-thorough to have a few test cases covering this obscure corner of behavior, detecting via the side effect that the value isn’t even converted to a string at all if the return value is already set to a non-empty string but that whether default is prevented has no effect, and also covering exactly what happens if the conversion to string throws an exception.

Regarding that last point, while this is not new to this patch, I just noticed that the code to call the event handler itself uses the version of JSC::profiledCall which explicitly extracts a thrown exception from the execution state. That code calls clearException to avoid leaving it behind in execution state, and then calls a function named uncaughtExceptionInEventHandler. Any exception from the toWTFString call we are making here is not handled that way. I am concerned about that. Seems unlikely to be 100% correct.

Another thought is that this special code for BeforeUnloadEvent is an unwanted distraction here in this JSEventListener code; it would be nicer to factor this out into a helper function, even though it’s just three lines of code. If it weren’t for the fact that we don’t really want to mix JavaScript binding code into the DOM, it would be nice if it was a virtual function so that this code didn’t even need to have a special case for BeforeUnloadEvent. But it’s probably more important to keep bindings separate from the DOM.

> Source/WebCore/loader/FrameLoader.cpp:3001
> +    // Only ask the user to confirm the navigation if the returnValue attribute of the
> +    // event object is not the empty string, or if the event was canceled.
> +    if (beforeUnloadEvent->returnValue().isEmpty() && !beforeUnloadEvent->defaultPrevented())
>          return true;

I am not thrilled with this comment. The comment says *what* the code does, but the code already says that. The comment does not say *why* it’s the right thing to do, and that’s what our comments should try to say. Maybe something more like:

    // Webpages indicate that confirmation is needed before leaving the page by setting the return
    // value of the event or by preventing default event handling, either by making direct calls on
    // the event or by returning a value from an event handler. If none of this was done, show no dialog.

My suggested comment is likely too long, but the point is that it talks about *why* this code is correct rather than stating what the code does. Another way to do this would be to write a named function called isConfirmationNeeded that takes a BeforeUnloadEvent& and put the comment in there. Then the code would say:

    if (!isConfirmationNeeded(*beforeUnloadEvent))
        return true;

And it would require no comment at all. The comment inside isConfirmationNeeded could be even clearer, then. Something like this:

    // Webpages either return a value from an event handler or make function calls directly on the
    // event to let us know that confirmation is needed; either providing a string or just preventing default.
    return !event.returnValue().isEmpty() || event.defaultPrevented();

[Chris Dumez]

Comment on attachment 301979 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +          explicitely been set by JS).
> 
> Typo here in the word explicitly.

Will fix. I always make this typo :/

>> Source/WebCore/bindings/js/JSEventListener.cpp:166
>> +                    beforeUnloadEvent.setReturnValue(retval.toWTFString(exec));
> 
> I believe that conversion to string can have an observable side effect, due to the fact that toPrimitive gets called on the value if it’s an object. Would be super-thorough to have a few test cases covering this obscure corner of behavior, detecting via the side effect that the value isn’t even converted to a string at all if the return value is already set to a non-empty string but that whether default is prevented has no effect, and also covering exactly what happens if the conversion to string throws an exception.
> 
> Regarding that last point, while this is not new to this patch, I just noticed that the code to call the event handler itself uses the version of JSC::profiledCall which explicitly extracts a thrown exception from the execution state. That code calls clearException to avoid leaving it behind in execution state, and then calls a function named uncaughtExceptionInEventHandler. Any exception from the toWTFString call we are making here is not handled that way. I am concerned about that. Seems unlikely to be 100% correct.
> 
> Another thought is that this special code for BeforeUnloadEvent is an unwanted distraction here in this JSEventListener code; it would be nicer to factor this out into a helper function, even though it’s just three lines of code. If it weren’t for the fact that we don’t really want to mix JavaScript binding code into the DOM, it would be nice if it was a virtual function so that this code didn’t even need to have a special case for BeforeUnloadEvent. But it’s probably more important to keep bindings separate from the DOM.

Regarding your first point. I believe you put your finger on a very subtle bug in our implementation. Because of the type of onBeforeUnloadEventHandler [1]. We should actually always convert the value to a String, except if the input is null or undefined. This is because the IDL type is a DOMString?.

[1] https://html.spec.whatwg.org/#onbeforeunloadeventhandler

>> Source/WebCore/loader/FrameLoader.cpp:3001
>>          return true;
> 
> I am not thrilled with this comment. The comment says *what* the code does, but the code already says that. The comment does not say *why* it’s the right thing to do, and that’s what our comments should try to say. Maybe something more like:
> 
>     // Webpages indicate that confirmation is needed before leaving the page by setting the return
>     // value of the event or by preventing default event handling, either by making direct calls on
>     // the event or by returning a value from an event handler. If none of this was done, show no dialog.
> 
> My suggested comment is likely too long, but the point is that it talks about *why* this code is correct rather than stating what the code does. Another way to do this would be to write a named function called isConfirmationNeeded that takes a BeforeUnloadEvent& and put the comment in there. Then the code would say:
> 
>     if (!isConfirmationNeeded(*beforeUnloadEvent))
>         return true;
> 
> And it would require no comment at all. The comment inside isConfirmationNeeded could be even clearer, then. Something like this:
> 
>     // Webpages either return a value from an event handler or make function calls directly on the
>     // event to let us know that confirmation is needed; either providing a string or just preventing default.
>     return !event.returnValue().isEmpty() || event.defaultPrevented();

Ok, I will integrate this.

[Chris Dumez]

Created attachment 302104 [details]
Patch

[Chris Dumez]

Comment on attachment 302104 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventListener.cpp:77
> +static void handleBeforeUnloadEventReturnValue(BeforeUnloadEvent& event, const String& returnValue)

Moved the logic to a separate function as suggested.

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

Make sure we cover the returnValue to a DOMString? no matter what, since this is observable. I also added a test to cover this.

> Source/WebCore/loader/FrameLoader.cpp:2978
> +static bool shouldAskForNavigationConfirmation(const BeforeUnloadEvent& event)

Moved this logic to a separate function as suggested.

> Source/WebCore/loader/FrameLoader.cpp:2980
> +    // Web pages can request we ask for confirmation before navigating by:

Improved the comment.

> LayoutTests/ChangeLog:14
> +        * fast/events/before-unload-return-string-conversion.html: Added.

New test for corner case found through review.

[Build Bot]

Comment on attachment 302104 [details]
Patch

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

New failing tests:
fast/events/before-unload-return-string-conversion.html

[Build Bot]

Created attachment 302112 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 302115 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 302115 [details]
Patch

Clearing flags on attachment: 302115

Committed r212625: <http://trac.webkit.org/changeset/212625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

The final patch looks great, but I have two small follow-up thoughts:

1) I think I would have put the conversion to a string into the helper function. I understand that it is critical to always do it, but I still think it’s easier to focus on the code when it’s in the helper function context rather than at the call site.

2) I’m not sure I understand what happens the the exception when the string conversion raises an exception. As I mentioned above, the actual call to the event handler is careful to clear the exception out of the VM if it occurs, but as far as I can tell the code for string conversion does not seem to be doing that; just lets the exception get set and ignores it but does not clear it out.

[Darin Adler]

what happens *to* the exception

[Anne van Kesteren]

*** Bug 169789 has been marked as a duplicate of this bug. ***