fix(common): canceled JSONP requests won't throw error with missing callback function #36807

martinsik · 2020-04-25T07:54:00Z

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Unsubscribing from a JSONP request will throw console errors such as Uncaught ReferenceError: ng_jsonp_callback_41 is not defined. This is happening because the callback function is removed right on unsubscription but this doesn't stop the JSONP script from loading.

https://stackblitz.com/edit/angular-4zmfjr?file=src/app/app.component.ts

What is the new behavior?

Unsubscribing from JSONP request won't remove callback handler. Handler is then removed by the handler itself:

angular/packages/common/http/src/jsonp.ts

Line 107 in 2365bb8

delete this.callbackMap[callback];

Does this PR introduce a breaking change?

Yes
No

martinsik · 2020-04-25T08:01:41Z

packages/common/http/test/jsonp_spec.ts

@@ -18,7 +18,6 @@ function runOnlyCallback(home: any, data: Object) {
  const keys = Object.keys(home);
  expect(keys.length).toBe(1);
  const callback = home[keys[0]];
-  delete home[keys[0]];


I removed this line because the handler should be removed by the handler itself:

angular/packages/common/http/src/jsonp.ts

Line 107 in 2365bb8

delete this.callbackMap[callback];

JoostK

It appears to be possible to prevent the script from executing by adopting the script element into another document, as described in https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block. So instead of removing the script element as we do now, it could instead be adopted into a dummy document. I think that doing so is beneficial in terms of resources, as it avoids the need to parse and execute the script.

packages/common/http/src/jsonp.ts

packages/common/http/test/jsonp_spec.ts

petebacondarwin · 2021-09-15T13:57:31Z

@martinsik - would you have time to address the comments from @JoostK?

martinsik · 2021-11-29T20:27:13Z

It's been so long I've completelly forgot I ever made this PR :).

@JoostK I made a small demo to test if changing <script>'s document will prevent it from exeuction and it does work https://stackblitz.com/edit/js-gs6nph?file=index.js
I refactored the code a little bit. Seems like one state variable (cancelled) could be removed completely and I shouldn't even need to use removeEventListener because the events won't be triggered after changing the owner document.

I'm not sure why ci/circleci: legacy-unit-tests-saucelab CICD process is failing but it looks like the same problem is in other PRs as well so it doesn't look like I broke it.

JoostK

Thanks for your continued effort on this PR. It looks good to me; I'll request a review from @alxhub as code owner of fw-http so we can get this landed.

packages/common/http/test/jsonp_spec.ts

JoostK · 2021-11-29T20:45:14Z

packages/common/http/test/jsonp_spec.ts

@@ -63,6 +62,11 @@ const SAMPLE_REQ = new HttpRequest<never>('JSONP', '/test');
      });
      document.mockError(error);
    });
+    it('allows the callback to be invoked when the request is cancelled', () => {
+      backend.handle(SAMPLE_REQ).subscribe().unsubscribe();
+      expect(Object.keys(home).length).toBe(0);


Is this sufficient to verify that the callback handler wasn't executed? Isn't this check only indicating that cleanup happened?

I updated the test to also check that the callback wasn't invoked.

packages/common/http/test/jsonp_spec.ts

JoostK · 2021-11-29T20:48:17Z

Btw don't worry about the Saucelabs failures; they have been very flaky over the last couple of weeks and #44281 should help make it more stable.

martinsik · 2022-01-12T09:29:06Z

@JoostK I updated this PR and implemented your comments.

AndrewKushnir · 2022-01-13T02:31:32Z

packages/common/http/src/jsonp.ts

+  private readonly dummyDocument = this.document.implementation.createHTMLDocument();
+


I'm wondering if it'd be better to create a document lazily as it's not always needed?

Something like this:

if (!finished) { // Issue #34818 // Changing <script>'s ownerDocument will prevent it from execution. // https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block if (!this.dummyDocument) { this.dummyDocument = this.document.implementation.createHTMLDocument(); } this.dummyDocument.adoptNode(node); }

Could you please also add a comment above the dummyDocument field to explain why it's needed?

I was also thinking if we can rename the field. May be something like foreignDocument?

Probably it'd also make sense to move that logic to a separate function, so it acts as an extra documentation as well:

// ... add docs here ... private removeListeners(node: HTMLElement): void { // Issue #34818 // Changing <script>'s ownerDocument will prevent it from execution. // https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block if (!this.dummyDocument) { this.dummyDocument = this.document.implementation.createHTMLDocument(); } this.dummyDocument.adoptNode(node); }

and after that:

if (!finished) { this.removeListeners(node); }

The benefit is that the main code path is straightforward (we just remove listeners) and the complexity is in the removeListeners function itself.

@AndrewKushnir I moved it into a separate method. I'm not sure if removeListeners isn't confusing because it's not really removing any listeners.

martinsik · 2022-02-14T11:31:16Z

@AndrewKushnir I rebased the code and implemented your suggestions.

AndrewKushnir

@martinsik the change looks good 👍

I'd like to ask @JoostK to take another look. After that we can run the necessary tests in Google's codebase and proceed with next steps if everything looks good.

Thank you.

JoostK · 2022-02-14T22:17:08Z

packages/common/http/src/jsonp.ts

+   * When a pending <script> is unsubscribed we'll move it to this document, so it won't be
+   * executed.
+   */
+  private foreignDocument!: Document;


nit: I'd personally move this to become a top-level variable, as this can be safely shared between multiple instances (although unlikely to be relevant except for tests) and it minimizes better.

I'd also request not to use ! here, as we can easily include |undefined in the type to make reads of this field null-safe. Or use Document|null = null; which produces more code but has the benefit that reads from this class stay monomorphic. But that wouldn't be a concern if it's a top-level var (nor is this performance sensitive code anyway, I'm mentioning it as a general takeaway).

tldr; I'd declare let foreignDocument: Document|undefined; as top-level declaration.

@JoostK I moved foreignDocument outside of the class, but it seems like I'll need to be using ! in foreignDocument!.adoptNode(script); because this.document is of type any so TypeScript can't know what type is returned from createHTMLDocument() .
I could use typecasting on this (this.document.implementation as DOMImplementation).createHTMLDocument() but that's probably not correct because document could use a different implementation.

Ah, interesting. If we're relying on createHTMLDocument to be available, then we might as well do the cast. If we don't want to rely on that then we'd need to account for the situation where implementaiton or createHTMLDocument is not available as we'd expect, but I don't think there's a reasonable alternative we could use in that case so that seems not viable to me.

Ok, I added typecast to DOMImplementation.

JoostK · 2022-02-23T20:17:08Z

@martinsik thanks, LGTM. Could you please rebase on latest master to get CI to run?

…issing callback function This commit fixes a use-case where unsubscribing from a JSONP request will result in "Uncaught ReferenceError: ng_jsonp_callback_xy is not defined" thrown into console. Unsubscribing won't remove its associated callback function because the requested script will finish loading anyway and will try to call the handler. PR Close angular#34818

handler Cancel pending json handler by adopting its <script> element into another document (https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block) This way the browser will prevent the script from being parsed and executed. Fixes angular#34818

jessicajaniuk · 2022-02-24T17:24:36Z

This PR was merged into the repository by commit 909b21a.

…6807) handler Cancel pending json handler by adopting its <script> element into another document (https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block) This way the browser will prevent the script from being parsed and executed. Fixes #34818 PR Close #36807

…issing callback function (#36807) This commit fixes a use-case where unsubscribing from a JSONP request will result in "Uncaught ReferenceError: ng_jsonp_callback_xy is not defined" thrown into console. Unsubscribing won't remove its associated callback function because the requested script will finish loading anyway and will try to call the handler. PR Close #34818 PR Close #36807

…6807) handler Cancel pending json handler by adopting its <script> element into another document (https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block) This way the browser will prevent the script from being parsed and executed. Fixes #34818 PR Close #36807

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.2.4/13.2.5) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.2.4/13.2.5) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.2.4/13.2.5) | | [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.2.4/13.2.5) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.2.4/13.2.5) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.2.4/13.2.5) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.2.4/13.2.5) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.2.4/13.2.5) | | [@angular/router](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.2.4/13.2.5) | | [zone.js](https://github.com/angular/angular) ([changelog](https://github.com/angular/angular/blob/master/packages/zone.js/CHANGELOG.md)) | dependencies | patch | [`0.11.4` -> `0.11.5`](https://renovatebot.com/diffs/npm/zone.js/0.11.4/0.11.5) | --- ### Release Notes <details> <summary>angular/angular (@angular/animations)</summary> ### [`v13.2.5`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1325-2022-03-02) [Compare Source](angular/angular@13.2.4...13.2.5) ##### animations | Commit | Type | Description | | -- | -- | -- | | [6c61d20476](angular/angular@6c61d20) | fix | allow animations with unsupported CSS properties ([#45185](angular/angular#45185)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [64da1daa78](angular/angular@64da1da) | fix | canceled JSONP requests won't throw console error with missing callback function ([#36807](angular/angular#36807)) | | [56ca7d385b](angular/angular@56ca7d3) | perf | make `NgLocalization` token tree-shakable ([#45118](angular/angular#45118)) ([#45226](angular/angular#45226)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [6c906a5bb9](angular/angular@6c906a5) | fix | Support resolve animation name from the DTS ([#45169](angular/angular#45169)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [e8fd452bd2](angular/angular@e8fd452) | fix | remove individual commands for updating gold files ([#45198](angular/angular#45198)) | | [82d772857c](angular/angular@82d7728) | perf | make `Compiler`, `ApplicationRef` and `ApplicationInitStatus` tree-shakable ([#45102](angular/angular#45102)) ([#45222](angular/angular#45222)) | | [71ff12c1cc](angular/angular@71ff12c) | perf | make `LOCALE_ID` and other tokens from `ApplicationModule` tree-shakable ([#45102](angular/angular#45102)) ([#45222](angular/angular#45222)) | ##### localize | Commit | Type | Description | | -- | -- | -- | | [d388522745](angular/angular@d388522) | fix | avoid imports into `compiler-cli` package ([#45180](angular/angular#45180)) | #### Special Thanks Andrew Kushnir, Andrew Scott, Charles Lyding, Guillaume Bonnet, Jessica Janiuk, JoostK, Martin Sikora, Paul Gschwendtner, Theodore Brown, dario-piotrowicz and ivanwonder  </details> <details> <summary>angular/angular (zone.js)</summary> ### [`v0.11.5`](https://github.com/angular/angular/blob/HEAD/packages/zone.js/CHANGELOG.md#0115-httpsgithubcomangularangularcomparezonejs-0114zonejs-0115-2022-03-03) [Compare Source](angular/angular@zone.js-0.11.4...zone.js-0.11.5) ##### Bug Fixes - **zone.js:** async-test should only call done once ([#45025](angular/angular#45025)) ([dea7234](angular/angular@dea7234)) - **zone.js:** defineProperties should also set symbol props ([#45098](angular/angular#45098)) ([b437d12](angular/angular@b437d12)), closes [#44095](angular/angular#44095) - **zone.js:** fix several test cases which trigger `done()` multiple times ([#45025](angular/angular#45025)) ([d5565cc](angular/angular@d5565cc)) - **zone.js:** only one listener should also re-throw an error correctly ([#41868](angular/angular#41868)) ([299f92c](angular/angular@299f92c)), closes [#41867](angular/angular#41867) [/github.com/angular/angular/pull/41562#issuecomment-822696973](https://github.com//github.com/angular/angular/pull/41562/issues/issuecomment-822696973) - **zone.js:** patch global instead of Mocha object ([#45047](angular/angular#45047)) ([8efbdb5](angular/angular@8efbdb5)), closes [#42834](angular/angular#42834) - **zone.js:** should continue to executue listeners when throw error ([#41562](angular/angular#41562)) ([008eaf3](angular/angular@008eaf3)), closes [#41522](angular/angular#41522) - **zone.js:** update several flaky cases ([#41526](angular/angular#41526)) ([25a83eb](angular/angular@25a83eb)), closes [#41434](angular/angular#41434) ##### Features - **zone.js:** add Promise.any() implementation ([#45064](angular/angular#45064)) ([4d494d2](angular/angular@4d494d2)), closes [#44393](angular/angular#44393) - **zone.js:** update electron patch to support electron/remote 14 ([#45073](angular/angular#45073)) ([d65706a](angular/angular@d65706a)), closes [#43346](angular/angular#43346) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1197 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2022-03-27T16:06:23Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Apr 25, 2020

pullapprove bot requested review from alxhub and mhevery Apr 25, 2020

martinsik force-pushed the issue-34818 branch from 67020ef to 9859ed7 Compare Apr 25, 2020

martinsik commented Apr 25, 2020

View changes

martinsik force-pushed the issue-34818 branch from 9859ed7 to d8530e3 Compare Apr 26, 2020

AndrewKushnir added the comp: common/http label Apr 29, 2020

ngbot bot added this to the needsTriage milestone Apr 29, 2020

petebacondarwin added action: review target: patch This PR is targeted for the next patch release labels Jan 28, 2021

ngbot bot modified the milestones: needsTriage, Backlog Jan 28, 2021

JoostK mentioned this pull request May 7, 2021

Canceling an in-progress JSONP request causes "callback undefined" console error #34818

Closed

JoostK reviewed May 7, 2021

View changes

packages/common/http/src/jsonp.ts Outdated Show resolved Hide resolved

packages/common/http/test/jsonp_spec.ts Outdated Show resolved Hide resolved

packages/common/http/test/jsonp_spec.ts Outdated Show resolved Hide resolved

mhevery removed their request for review Jun 11, 2021

JoostK added action: cleanup and removed action: review labels Jul 17, 2021

martinsik force-pushed the issue-34818 branch 3 times, most recently from 656fa16 to 20c4c8a Compare Nov 25, 2021

JoostK reviewed Nov 29, 2021

View changes

JoostK added action: review and removed action: cleanup labels Nov 29, 2021

martinsik force-pushed the issue-34818 branch 2 times, most recently from 920e501 to 47d1168 Compare Dec 21, 2021

JoostK requested a review from AndrewKushnir Jan 12, 2022

AndrewKushnir reviewed Jan 13, 2022

View changes

martinsik force-pushed the issue-34818 branch from 47d1168 to 91ca2ed Compare Feb 9, 2022

AndrewKushnir approved these changes Feb 14, 2022

View changes

JoostK approved these changes Feb 14, 2022

View changes

JoostK added the action: cleanup label Feb 14, 2022

martinsik force-pushed the issue-34818 branch 2 times, most recently from 6da6cc6 to aa9a76d Compare Feb 23, 2022

martinsik added 2 commits Feb 23, 2022

martinsik force-pushed the issue-34818 branch from aa9a76d to 78aaf76 Compare Feb 23, 2022

JoostK added action: merge PR author is ready for this to merge action: presubmit A standard presubmit is running / required and removed action: cleanup action: review labels Feb 23, 2022

jessicajaniuk removed the request for review from alxhub Feb 24, 2022

jessicajaniuk removed the action: presubmit A standard presubmit is running / required label Feb 24, 2022

jessicajaniuk closed this in 7671a1e Feb 24, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 27, 2022

fix(common): canceled JSONP requests won't throw error with missing callback function #36807

fix(common): canceled JSONP requests won't throw error with missing callback function #36807

martinsik commented Apr 25, 2020 •

edited by JoostK

martinsik Apr 25, 2020

JoostK left a comment

petebacondarwin commented Sep 15, 2021

martinsik commented Nov 29, 2021

JoostK left a comment

JoostK Nov 29, 2021

martinsik Dec 21, 2021

JoostK commented Nov 29, 2021

martinsik commented Jan 12, 2022

AndrewKushnir Jan 13, 2022

AndrewKushnir Jan 13, 2022

martinsik Feb 14, 2022

martinsik commented Feb 14, 2022

AndrewKushnir left a comment

JoostK Feb 14, 2022

martinsik Feb 23, 2022

JoostK Feb 23, 2022

martinsik Feb 23, 2022

JoostK commented Feb 23, 2022

jessicajaniuk commented Feb 24, 2022

angular-automatic-lock-bot bot commented Mar 27, 2022

		private readonly dummyDocument = this.document.implementation.createHTMLDocument();

fix(common): canceled JSONP requests won't throw error with missing callback function #36807

fix(common): canceled JSONP requests won't throw error with missing callback function #36807

Conversation

martinsik commented Apr 25, 2020 • edited by JoostK

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

JoostK left a comment

petebacondarwin commented Sep 15, 2021

martinsik commented Nov 29, 2021

JoostK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK commented Nov 29, 2021

martinsik commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinsik commented Feb 14, 2022

AndrewKushnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK commented Feb 23, 2022

jessicajaniuk commented Feb 24, 2022

angular-automatic-lock-bot bot commented Mar 27, 2022

martinsik commented Apr 25, 2020 •

edited by JoostK