Don't cancel Events with type beforeunload [chromium/src : master]

Rakina Zata Amni (Gerrit)

unread,

Oct 19, 2017, 6:56:40 AM10/19/17

to Yuki Shiino, Takayoshi Kochi, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, chromium...@chromium.org, Commit Bot

Rakina Zata Amni uploaded patch set #6 to this change.

View Change

Don't cancel Events with type beforeunload

If we return the boolean false in a beforeunload eventlistener, the
unload event will get canceled. This is not per-spec, which is in
https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm 

This is because the return value of the event listener is not converted
to DOMString, because OnBeforeUnloadEventHandler is not currently
implemented. See:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/WindowEventHandlers.idl?q=beforeunloadeventhandler&dr=C&l=39

This is a temporary fix until OnBeforeUnloadEventHandler is
implemented, making sure that Events with the type beforeunload
won't get canceled (in accordance to the spec). 

Bug: 692203
Change-Id: Ic31e8b90fb2edad173e99ac6a7d9059162a53278
---
M third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt
M third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp
2 files changed, 8 insertions(+), 2 deletions(-)

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Rakina Zata Amni (Gerrit)

unread,

Oct 19, 2017, 6:58:41 AM10/19/17

to blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Yuki Shiino, Commit Bot, chromium...@chromium.org

View Change

2 comments:

Commit Message:
- Patch Set #5, Line 9: If we return the boolean false in a beforeunload eventlistener, the
  As the description will be used as is in git's commit message, please […]
  Done
- Patch Set #5, Line 13: This is because the return value of the event listener is not converted
  By "temporary" you mean until OnBeforeUnloadEventHandler is implemented? […]
  Sorry, yep I meant any Event with the type beforeunload, and added information about the "temporary part"

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Yuki Shiino (Gerrit)

unread,

Oct 19, 2017, 9:00:10 AM10/19/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:
- Patch Set #4, Line 181: (event->IsBeforeUnloadEvent() ||
  So, that part of the spec is implemented correctly right now (as far as my understanding, might be w […]
  It's definitely better to do refactoring here about the step 5. Please refactor the code from line 170 to line 182 (the end of this function).
```
Also note that, IIUC, the statement in the spec "If E is a BeforeUnloadEvent object and E's type is beforeunload" does NOT mean:
    case a) E is a BeforeUnloadEvent object,
    case b) E's type is beforeunload
    and if either of case a) or b), ...
I understand the statement as
    condition a) E is a BeforeUnloadEvent object,
    condition b) E's type is beforeunload
    and if both of the conditions a) and b) are satisfied, ...
```
  If both cases are okay, then the statement must be "If case a) OR case b), ..." instead of "If condition a) AND condition b), ..."

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Rakina Zata Amni (Gerrit)

unread,

Oct 19, 2017, 9:35:13 AM10/19/17

to blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Yuki Shiino, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:

- Patch Set #4, Line 181: (event->IsBeforeUnloadEvent() ||

- It's definitely better to do refactoring here about the step 5. […]
  OK, will do. Should I put the code for the whole step 5 (so, applying the code for all events) or should it be specifically just for Events with the type beforeunload and BeforeUnloadEvents?
  Also, yeah, I understand the spec the same way as you (the second interpretation you mentioned).
  What I meant by the "step 5 check for BeforeUnloadEvent or events with type beforeunload" that I mentioned in the previous comment was the first case and the last ("otherwise") case of the step 5. The first case will take BeforeUnloadEvent that has the type beforeunload, the last case will take events that are of the type beforeunload but isn't a BeforeUnloadEvent :D. Sorry for the bad wording!

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Yuki Shiino (Gerrit)

unread,

Oct 19, 2017, 11:10:08 AM10/19/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:

- Patch Set #4, Line 181: (event->IsBeforeUnloadEvent() ||

- OK, will do. Should I put the code for the whole step 5 (so, applying the code for all events) or should it be specifically just for Events with the type beforeunload and BeforeUnloadEvents?
  Either will do. It's great to fix things around, but it's also okay to focus on beforeunload.

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Yuki Shiino (Gerrit)

unread,

Oct 19, 2017, 11:29:12 AM10/19/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:

- Patch Set #4, Line 181: (event->IsBeforeUnloadEvent() ||

- > OK, will do. […]
  Let me clarify my point.
```
IIUC, this CL doesn't handle a case that
  E is a BeforeUnloadEvent object, and
  E's type is beforeunload, and
  the return value is not null
In this case, we need to do preventDefault (= set E's cancel flag).  However, this CL seems not cancelling it.
```
  So, I think that we need to do some refactoring around here just in order to fix beforeunload event. I'm fine to ignore "special error event handling" case. It seems not implemented here. I vaguely remember that we have "special error event handling" case's code somewhere else.

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Rakina Zata Amni (Gerrit)

unread,

Oct 20, 2017, 3:12:47 AM10/20/17

to blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Yuki Shiino, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:
- Patch Set #4, Line 181: ToBeforeUnloadEvent(event)->returnValue().IsEmpty()) {
  Let me clarify my point. […]
  I did the refactoring (and now all the test passes!). PTAL :D

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Takayoshi Kochi (Gerrit)

unread,

Oct 20, 2017, 3:34:46 AM10/20/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Yuki Shiino, Commit Bot, chromium...@chromium.org

View Change

2 comments:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:
- Patch Set #7, Line 181: ToBeforeUnloadEvent(event)->returnValue().IsEmpty()) {
  IsEmpty() is not necessary - IsEmpty() is checking if it is "".
  (WTF String's IsEmpty() seems implying either IsNull() or "")
- Patch Set #7, Line 185: }
  I think this function should return from here, you already event->preventDefault() for
  this case on line 179.

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Yuki Shiino (Gerrit)

unread,

Oct 20, 2017, 3:36:48 AM10/20/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

3 comments:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:
- Patch Set #8, Line 169:
  If |!is_attribute|, there is nothing we'd like to do. So let's return early at this point. If it turned out that we need to handle the |!is_attribute| case in the same way, we can simply remove the early return in another CL.
- Patch Set #8, Line 181: ToBeforeUnloadEvent(event)->returnValue().IsEmpty()) {
  What we'd like to do here is conceptually:
```
  |event|'s return value = either of
      null or
      undefined or
      string (e.g. ToString(return_value) )
```
  This code looks a little different. What we need to do are:
```
  a) Convert |return_value| to one of null, undefined or string.
  b) Store the converted return value into event->returnValue().
```
  I'm not sure what the value of event->returnValue() would be if we didn't perform setReturnValue, so I'd recommend to always call event->setReturnValue even if the return value is null or undefined.
- Patch Set #8, Line 188: if (is_attribute_ && ShouldPreventDefault(return_value) &&
  I'd prefer |else if| here, then we no longer need to think about the complicated beforeunload condition.
  Or "early return" on line 186 makes sense to me.

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Rakina Zata Amni (Gerrit)

unread,

Oct 20, 2017, 4:05:35 AM10/20/17

to blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Yuki Shiino, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

View Change

5 comments:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:
- Patch Set #7, Line 181: ToBeforeUnloadEvent(event)->returnValue().IsEmpty()) {
  IsEmpty() is not necessary - IsEmpty() is checking if it is "". […]
  Whoops yeah didn't notice that. Done :)

- Patch Set #7, Line 185: }

- I think this function should return from here, you already event->preventDefault() for […]
  Done (returned early at the top instead)
File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:

- Patch Set #8, Line 169:

- If |!is_attribute|, there is nothing we'd like to do. So let's return early at this point. […]
  Done

- Patch Set #8, Line 181: ToBeforeUnloadEvent(event)->returnValue().IsEmpty()) {

- What we'd like to do here is conceptually: […]
  So, event->returnValue() actually can be set in the listener function and maybe different than the return value of the listener function. The spec says to only assign event->returnValue = return_value when it's not set/empty.
  The test case here shows the scenario where event's returnValue should be different than the listener function's return value:
  https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html?sq=package:chromium&l=138
  https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html?l=21

- Patch Set #8, Line 188: if (is_attribute_ && ShouldPreventDefault(return_value) &&

- I'd prefer |else if| here, then we no longer need to think about the complicated beforeunload condit […]
  Done

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Yuki Shiino (Gerrit)

unread,

Oct 20, 2017, 4:35:05 AM10/20/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Commit Bot, chromium...@chromium.org

LGTM!

Patch set 9:Code-Review +1

View Change

1 comment:

File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:
- Patch Set #8, Line 181: if (event->IsBeforeUnloadEvent() &&
  So, event->returnValue() actually can be set in the listener function and maybe different than the r […]
  Good to know. You're right.

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Takayoshi Kochi (Gerrit)

unread,

Oct 20, 2017, 5:06:42 AM10/20/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Yuki Shiino, Commit Bot, chromium...@chromium.org

LGTM

Patch set 9:Code-Review +1

View Change

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Rakina Zata Amni (Gerrit)

unread,

Oct 20, 2017, 5:11:42 AM10/20/17

to blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Yuki Shiino, Commit Bot, chromium...@chromium.org

Patch set 9:Commit-Queue +2

View Change

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Oct 20, 2017, 6:34:49 AM10/20/17

to Rakina Zata Amni, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, Takayoshi Kochi, Yuki Shiino, chromium...@chromium.org

Commit Bot merged this change.

View Change

Approvals:
  Takayoshi Kochi: Looks good to me
  Yuki Shiino: Looks good to me
  Rakina Zata Amni: Commit

Don't cancel Events with type beforeunload

If we return the boolean false in a beforeunload eventlistener, the

unload event will get canceled. This is not per-spec, which is in
https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm

This is because the return value of the event listener is not converted

to DOMString, because OnBeforeUnloadEventHandler is not currently
implemented. See:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/WindowEventHandlers.idl?q=beforeunloadeventhandler&dr=C&l=39

This is a temporary fix until OnBeforeUnloadEventHandler is
implemented, making sure that Events with the type beforeunload
won't get canceled (in accordance to the spec). 

Bug: 692203
Change-Id: Ic31e8b90fb2edad173e99ac6a7d9059162a53278

Reviewed-on: https://chromium-review.googlesource.com/727863
Reviewed-by: Yuki Shiino <yukis...@chromium.org>
Reviewed-by: Takayoshi Kochi <ko...@chromium.org>
Commit-Queue: Rakina Zata Amni <rak...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510398}
---
D third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt
M third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp
2 files changed, 21 insertions(+), 24 deletions(-)

To view, visit change 727863. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward