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

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:39:27Z

packages/common/http/test/jsonp_spec.ts

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:24:28Z

packages/common/http/src/jsonp.ts

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:14:09Z

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

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 April 25, 2020 07:54

martinsik force-pushed the issue-34818 branch from 67020ef to 9859ed7 Compare April 25, 2020 07:59

martinsik commented Apr 25, 2020

View reviewed changes

martinsik force-pushed the issue-34818 branch from 9859ed7 to d8530e3 Compare April 26, 2020 15:51

AndrewKushnir added the area: common/http Issues related to HTTP and HTTP Client label Apr 29, 2020

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

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer 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 reviewed 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 June 11, 2021 18:47

JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 17, 2021

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

JoostK reviewed Nov 29, 2021

View reviewed changes

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 29, 2021

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

JoostK requested a review from AndrewKushnir January 12, 2022 10:21

AndrewKushnir reviewed Jan 13, 2022

View reviewed changes

martinsik force-pushed the issue-34818 branch from 47d1168 to 91ca2ed Compare February 9, 2022 21:54

AndrewKushnir approved these changes Feb 14, 2022

View reviewed changes

JoostK approved these changes Feb 14, 2022

View reviewed changes

JoostK added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 14, 2022

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

martinsik added 2 commits February 23, 2022 21:18

martinsik force-pushed the issue-34818 branch from aa9a76d to 78aaf76 Compare February 23, 2022 20:18

jessicajaniuk removed the request for review from alxhub February 24, 2022 00:35

jessicajaniuk removed the action: presubmit The PR is in need of a google3 presubmit 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

Uh oh!

Conversation

martinsik commented Apr 25, 2020 • edited by JoostK Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Sep 15, 2021

Uh oh!

martinsik commented Nov 29, 2021

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoostK commented Nov 29, 2021

Uh oh!

martinsik commented Jan 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinsik commented Feb 14, 2022

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK commented Feb 23, 2022

Uh oh!

jessicajaniuk commented Feb 24, 2022

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

martinsik commented Apr 25, 2020 •

edited by JoostK

Loading