Patch strip custom ns attrs issue 14928 #15030
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
| result += element; | ||
| } | ||
| expectHTML('a' + result + 'c').toEqual('a' + result + 'c'); | ||
| }); |
There was a problem hiding this comment.
So, is this testcase failing in IE without the fix? What version of IE?
|
That's correct. I was concerned about making sure the test was valid, so I tested the test. This test failed on IE 11 on Win10 Anniversary 64 bit with the expected "Out of stack space" error. This test did not fail on Chrome or FireFox. The updated version passes on all three. I did not test against IE 10 or IE 9 nor on any other OSes for IE. As for my CLA, this has happened to me before. I am the only commit author, my CLA has been manually verified before #14221 Comment |
|
LGTM We need to sort out the CLA issue, though. In the other PR, |
|
@BobChao87 can you make sure that the email of the commit author matches an email you have set in Github? |
|
I've looked around, and I'm really not sure how this came to be. The computer I had previously committed on broke recently and this was committed from a virtual machine on my new computer. It looks like Fedora was "helpful" and used my name registered with the OS as the commit name, despite using the |
|
If new commits won't have this problem you should be able to amend the
commit with your new email and push force again.
|
|
@BobChao87 Can you fix the commit or open a new PR with these changes (but a commit author that matches the Github email)? |
|
Yes, sorry, I've been busy. Hopefully by end of day tomorrow I'll have that for you. |
|
Hopefully that will be sufficient. If that doesn't work, I'll make a new branch and submit a new pull. |
|
@BobChao87 You need to replace the previous commits after you have changed the author / reauthored them. So remove the first 3 commits from your branch and then push --force the branch again. |
BobChao87
force-pushed
the
patch-stripCustomNsAttrs-issue-14928
branch
from
c87d5c7 to
a272a3c
Compare
Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928
|
Removed old commits and re-added as proper Github account. |
|
CLAs look good, thanks! |
|
CLAs look good, thanks! |
2 similar comments
|
CLAs look good, thanks! |
|
CLAs look good, thanks! |
|
Looking at the Travis build that failed, it seems like iOS had some unrelated issues involving sockets and |
|
rebuilding |
Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928 Closes angular#15030
Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928 Closes angular#15030
Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928 Closes angular#15030
Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928 Closes angular#15030
Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928 Closes angular#15030
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
What is the current behavior? (You can also link to an open issue here)
Issue #14928.
Summary: IE has a stack overflow on large datasets being parsed through
$sanitize.What is the new behavior (if this is a feature change)?
Changes the loop to leave fewer remnant function calls by performing an in-function while loop over the sibling elements being processed. Ensuring only one function on the stack for each level of depth.
Does this PR introduce a breaking change?
No. Unless someone is somehow using a fatal JS execution to detect IE. I'm sure there are better ways.
Please check if the PR fulfills these requirements
Other information:
Addresses issue by reducing the stack size, but not by the maximal possible amount. It was decided that instead of doing a fully in-function walk of the tree, it was sufficient to deal with the horizontal issue as most use cases don't have an extremely deep (ca. thousands) DOM structure.