Bug 204912 – Prevent synchronous XHR in beforeunload / unload event handlers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Prevent synchronous XHR in beforeunload / unload event handlers. They are terrible for performance and the Beacon API (or Fetch keepalive) are more efficient & supported alternatives.

[Radar WebKit Bug Importer]

<rdar://problem/57676394>

[Chris Dumez]

Created attachment 384955 [details]
Patch

[Chris Dumez]

Created attachment 384970 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 384970 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:134
> +    if (!m_async && (!document.page() || !document.page()->areSynchronousLoadsAllowed())) {

Seems like we want to put this behind some kind of a setting / feature flag?

[Ryosuke Niwa]

Do other browsers forbid this as well?

[Sam Weinig]

It seems like if we want to make this kind of change, we should first understand the compatibility story and if it's ok, make a change to HTML5 to standardize this.

Otherwise, it's not clear why this is an ok change to make.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #5)
> Do other browsers forbid this as well?

No, other browsers do not do this at this time.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #5)
> Do other browsers forbid this as well?

Please see https://www.chromestatus.com/feature/4664843055398912

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Ryosuke Niwa from comment #5)
> > Do other browsers forbid this as well?
> 
> No, other browsers do not do this at this time.

Sorry, I meant the opposite. Not sure what happened there:

Chrome has been testing this for a while:
https://www.chromestatus.com/feature/4664843055398912

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 384970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384970&action=review
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:134
> > +    if (!m_async && (!document.page() || !document.page()->areSynchronousLoadsAllowed())) {
> 
> Seems like we want to put this behind some kind of a setting / feature flag?

Good idea, will do.

[Chris Dumez]

Created attachment 385015 [details]
Patch

[Chris Dumez]

Created attachment 385025 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 385025 [details]
Patch

Clearing flags on attachment: 385025

Committed r253213: <https://trac.webkit.org/changeset/253213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.