From 14a30515523c7b55800ec5af8c651fb5c68d3551 Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Mon, 26 Jun 2023 22:58:08 +0000 Subject: [PATCH] [dart:html] Move NullWindowException to implementation Window.open may open a null window in more cases than expected. Users may not care that the window they get back is invalid if they never use it. Therefore, this CL moves the exception to the implementation of the returned window in order to reduce noise, but still give a way for users to recover if they wish to do so. CoreLibraryReviewExempt: Backend-specific library. Change-Id: I005cf80630cfb4db2f5ec2012cfcd0161ad10ff1 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/311460 Commit-Queue: Srujan Gaddam Reviewed-by: Sigmund Cherem --- CHANGELOG.md | 8 -------- sdk/lib/html/dart2js/html_dart2js.dart | 14 ++++++++------ tests/lib/html/window_test.dart | 13 ++++++++----- tests/lib_2/html/window_test.dart | 12 ++++++++++++ tools/dom/src/dart2js_DOMImplementation.dart | 9 +++++++-- .../templates/html/impl/impl_Window.darttemplate | 5 +---- 6 files changed, 36 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8e1bfbb6d3..522154c9247 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,14 +36,6 @@ [#51486]: https://github.com/dart-lang/sdk/issues/51486 [#52027]: https://github.com/dart-lang/sdk/issues/52027 -#### `dart:html` - -- **Breaking change to Window.open**: - `Window.open` will now throw an exception that can be caught - (`NullWindowException`) if the opened window is null. Previously, this null - window would be wrapped, and there would be surprising runtime errors when any - member is used on the wrapper. - #### `dart:js_interop` - **Object literal constructors**: diff --git a/sdk/lib/html/dart2js/html_dart2js.dart b/sdk/lib/html/dart2js/html_dart2js.dart index e8587eb2b35..e4f5fbfef10 100644 --- a/sdk/lib/html/dart2js/html_dart2js.dart +++ b/sdk/lib/html/dart2js/html_dart2js.dart @@ -32154,8 +32154,6 @@ class Window extends EventTarget /** * Opens a new window. * - * Throws a NullWindowException if the opened window is null. - * * ## Other resources * * * [Window.open](https://developer.mozilla.org/en-US/docs/Web/API/Window.open) @@ -32164,7 +32162,6 @@ class Window extends EventTarget WindowBase open(String url, String name, [String? options]) { final win = options == null ? _open2(url, name) : _open3(url, name, options); - if (win == null) throw new NullWindowException(); return _DOMWindowCrossFrame._createSafe(win); } @@ -33782,7 +33779,7 @@ class Window extends EventTarget class NullWindowException implements Exception { @override String toString() { - return 'Attempted to call Window.open with a null window.'; + return 'Attempting to use a null window opened in Window.open.'; } } // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file @@ -40124,7 +40121,12 @@ class _DOMWindowCrossFrame implements WindowBase { // Private window. Note, this is a window in another frame, so it // cannot be typed as "Window" as its prototype is not patched // properly. Its fields and methods can only be accessed via JavaScript. - final Object _window; + final Object? __window; + + Object get _window { + if (__window == null) throw new NullWindowException(); + return __window!; + } // Fields. HistoryBase get history => @@ -40161,7 +40163,7 @@ class _DOMWindowCrossFrame implements WindowBase { } // Implementation support. - _DOMWindowCrossFrame(this._window); + _DOMWindowCrossFrame(this.__window); static WindowBase _createSafe(w) { if (identical(w, window)) { diff --git a/tests/lib/html/window_test.dart b/tests/lib/html/window_test.dart index 2a3fc319527..9e6ff6a1bb3 100644 --- a/tests/lib/html/window_test.dart +++ b/tests/lib/html/window_test.dart @@ -12,12 +12,15 @@ main() { expect(window.scrollY, 0); }); test('open', () { - window.open('', 'blank'); + final valid = window.open('', 'blank'); + valid.closed; + // A blank page with no access to the original window (noopener) should + // result in null. + final invalid = window.open('', 'invalid', 'noopener=true'); try { - // A blank page with no access to the original window (noopener) should - // result in null. - window.open('', 'invalid', 'noopener=true'); - fail('Expected Window.open to throw.'); + // Should result in an exception since the underlying window is null. + invalid.closed; + fail('Expected invalid.closed to throw.'); } on NullWindowException {} }); } diff --git a/tests/lib_2/html/window_test.dart b/tests/lib_2/html/window_test.dart index 2aa67fcdc39..154cba6fe8a 100644 --- a/tests/lib_2/html/window_test.dart +++ b/tests/lib_2/html/window_test.dart @@ -13,4 +13,16 @@ main() { expect(window.scrollX, 0); expect(window.scrollY, 0); }); + test('open', () { + final valid = window.open('', 'blank'); + valid.closed; + // A blank page with no access to the original window (noopener) should + // result in null. + final invalid = window.open('', 'invalid', 'noopener=true'); + try { + // Should result in an exception since the underlying window is null. + invalid.closed; + fail('Expected invalid.closed to throw.'); + } on NullWindowException {} + }); } diff --git a/tools/dom/src/dart2js_DOMImplementation.dart b/tools/dom/src/dart2js_DOMImplementation.dart index 29ccba85ded..3fe4c7ff6fa 100644 --- a/tools/dom/src/dart2js_DOMImplementation.dart +++ b/tools/dom/src/dart2js_DOMImplementation.dart @@ -9,7 +9,12 @@ class _DOMWindowCrossFrame implements WindowBase { // Private window. Note, this is a window in another frame, so it // cannot be typed as "Window" as its prototype is not patched // properly. Its fields and methods can only be accessed via JavaScript. - final Object _window; + final Object? __window; + + Object get _window { + if (__window == null) throw new NullWindowException(); + return __window!; + } // Fields. HistoryBase get history => @@ -46,7 +51,7 @@ class _DOMWindowCrossFrame implements WindowBase { } // Implementation support. - _DOMWindowCrossFrame(this._window); + _DOMWindowCrossFrame(this.__window); static WindowBase _createSafe(w) { if (identical(w, window)) { diff --git a/tools/dom/templates/html/impl/impl_Window.darttemplate b/tools/dom/templates/html/impl/impl_Window.darttemplate index d270c55a280..0df2cd773c5 100644 --- a/tools/dom/templates/html/impl/impl_Window.darttemplate +++ b/tools/dom/templates/html/impl/impl_Window.darttemplate @@ -50,8 +50,6 @@ $(CLASS_MODIFIERS)class $CLASSNAME$EXTENDS$IMPLEMENTS { /** * Opens a new window. * - * Throws a NullWindowException if the opened window is null. - * * ## Other resources * * * [Window.open](https://developer.mozilla.org/en-US/docs/Web/API/Window.open) @@ -60,7 +58,6 @@ $(CLASS_MODIFIERS)class $CLASSNAME$EXTENDS$IMPLEMENTS { WindowBase open(String url, String name, [String$NULLABLE options]) { final win = options == null ? _open2(url, name) : _open3(url, name, options); - if (win == null) throw new NullWindowException(); return _DOMWindowCrossFrame._createSafe(win); } @@ -259,6 +256,6 @@ $!MEMBERS class NullWindowException implements Exception { @override String toString() { - return 'Attempted to call Window.open with a null window.'; + return 'Attempting to use a null window opened in Window.open.'; } }