Selector: Use shallow document comparisons to avoid IE/Edge crashes #4471
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgol
force-pushed
the
ie-edge-traversing-crash
branch
from
686f2c7 to
aa710a0
Compare
mgol
commented
Aug 26, 2019
src/selector.js
Outdated
| // IE/Edge sometimes throw a "Permission denied" error when strict-comparing | ||
| // two documents; shallow comparisons work. | ||
| // eslint-disable-next-line eqeqeq | ||
| if ( ( context ? context.ownerDocument || context : preferredDoc ) != document ) { |
There was a problem hiding this comment.
This line is the only place where I've seen IE/Edge test failures. I thought, though, that it may be worth changing all document comparisons to make sure we're not missing some potential errors.
I could also create an internal utility function to compare documents & put the comment there. That would most likely increase the size, though (currently this PR is at -5 bytes).
There was a problem hiding this comment.
A version with an utility function would be +15 bytes instead of -5: mgol@53d0840.
|
I'll submit a Sizzle PR after we have consensus here. |
mgol
commented
Aug 26, 2019
| if ( doc === document || doc.nodeType !== 9 ) { | ||
| // Support: IE 11+, Edge 17 - 18+ | ||
| // IE/Edge sometimes throw a "Permission denied" error when strict-comparing | ||
| // two documents; shallow comparisons work. |
There was a problem hiding this comment.
If we want to go for this solution, do you think we should avoid this duplicate comment in some way?
|
What's funny is that I can reproduce the issue easily (not 100% of times but pretty often) in IE 11 & Edge 18 & I assume by TestSwarm failures that it's reproducible in Edge 17 as well - but I cannot reproduce it in IE 9 or 10. Seems they regressed? |
…crashes IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. Fixes jquerygh-4441
mgol
force-pushed
the
ie-edge-traversing-crash
branch
from
aa710a0 to
6b1c6a7
Compare
timmywil
approved these changes
Sep 3, 2019
|
I still approve. |
mgol
changed the title
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 24, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. The change to shallow comparisons in `src/selector.js` was done in jquerygh-4471 but relevant changes in `src/selector/uniqueSort.js` were missed. Fixes jquerygh-4441 Closes jquerygh-4471
mgol
added a commit
to mgol/sizzle
that referenced
this pull request
Sep 26, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. Fixes jquery/jquery#4441 Closes jquery/jquery#4471
mgol
added a commit
to mgol/sizzle
that referenced
this pull request
Sep 30, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. Fixes jquery/jquery#4441 Closes jquery/jquery#4471
mgol
added a commit
to mgol/sizzle
that referenced
this pull request
Oct 1, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. Fixes jquery/jquery#4441 Closes jquery/jquery#4471
mgol
added a commit
to jquery/sizzle
that referenced
this pull request
Oct 1, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. Fixes jquery/jquery#4441 Closes gh-459 Ref jquery/jquery#4471
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Oct 9, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. The change to shallow comparisons in `src/selector.js` was done in jquerygh-4471 but relevant changes in `src/selector/uniqueSort.js` were missed. Fixes jquerygh-4441 Closes jquerygh-4471
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Oct 14, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. The change to shallow comparisons in `src/selector.js` was done in jquerygh-4471 but relevant changes in `src/selector/uniqueSort.js` were missed. Fixes jquerygh-4441 Closes jquerygh-4471
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Oct 14, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. The change to shallow comparisons in `src/selector.js` was done in jquerygh-4471 but relevant changes in `src/selector/uniqueSort.js` were missed. Those changes have landed in Sizzle in jquery/sizzle#459. Fixes jquerygh-4441 Closes jquerygh-4471 Ref jquery/sizzle#459
4 tasks
mgol
added a commit
that referenced
this pull request
Oct 21, 2019
IE/Edge sometimes crash when comparing documents between frames using the strict equality operator (`===` & `!==`). Funnily enough, shallow comparisons (`==` & `!=`) work without crashing. The change to shallow comparisons in `src/selector.js` was done in gh-4471 but relevant changes in `src/selector/uniqueSort.js` were missed. Those changes have landed in Sizzle in jquery/sizzle#459. Fixes gh-4441 Closes gh-4512 Ref gh-4471 Ref jquery/sizzle#459
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
IE/Edge sometimes crash when comparing documents between frames using the strict
equality operator (
===&!==). Funnily enough, shallow comparisons(
==&!=) work without crashing.Fixes gh-4441
While I cannot be 100% certain this PR will solve the issue, I modified the code locally, wrapping the place where the browser crashes in a try-catch where I assigned both compared documents to global variables. I then compared them using all 4 operators manually in the console and the strict one were always crashing. I reproduced the same behavior in IE 11 & Edge 18.
I can't really write a test for it as the crashes are not consistent - the only consistent thing was that if it crashed once on a particular comparison, it keeps crashing on strict-comparing those two documents.
This fixes a failure in traversing tests but the code is in Selector so I'm marking the component as such.
Checklist
New tests have been added to show the fix or feature worksNot possible to test it reliably IMOIf needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com