fix(ngSanitize): prevent decodeEntities from prepending original text… #5193
Conversation
|
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
|
Oh this is fun, Safari 7 fails all kinds of tests apparently with origin/master (https://gist.github.com/caitp/7699674) --- But passes everything with this change. Weirdness. (the one travis failure |
| if (parts[2]) { | ||
| hiddenPre.innerHTML=parts[2].replace(/</g,"<"); | ||
| parts[2] = hiddenPre.innerText || hiddenPre.textContent; | ||
| } | ||
| parts[0] = ''; |
There was a problem hiding this comment.
I can't really explain why moving this assignment has that strong of an effect --- the VM is doing some weird things. But it does fix the problem
|
Thanks Caitlin ! |
… to output Clearing the match before the conditional branch seems to (inexplicably) break Safari 7 on OSX 10.9, except when stepping through line by line. Moving the assignment to after the conditional branch corrects this behaviour.
|
For reference, this was also an issue on Ubuntu 13.04 and 13.10, so if you can't add OSX to the CI servers this platform would have caught it. (EDIT: Actually, it was only a problem with QtWebKit on Ubuntu -- so an older WebKit -- harder to test. Sorry for the false hopes!) |
|
I wonder if the result of executing the regex is not strictly an array. So assigning to it changes things in unexpected ways |
|
I'm not totally sure, during debugging it looks totally normal --- it seems like an optimization error or something, where the conditional block and join decide to happen before the assignment happens or something. But I'm really just guessing at what it's doing, there might be a bug open on the webkit issue tracker regarding it already, or if not, maybe there should be. |
|
I filed a bug on webkit on friday regarding this, because I'm fairly sure this is not the expected behaviour. So just marking this here for future reference: https://bugs.webkit.org/show_bug.cgi?id=125023 |
|
Good work Caitlin, it's interesting to follow up and see what is the cause for this strange behavior... |
ghost
assigned
|
|
|
There is definitely something wrong with Safari 7 regex. Have a look at debuggex.com - start typing into the box and after a while it starts adding spaces to the end of your text. If you try to delete the spaces then it deletes the letters instead. |
|
Here is a minimal reproduction. http://plnkr.co/edit/CvKkFkvga8Pdz7cik3vf?p=preview |
|
Looks like Safari has started using the YARR regex engine, which JITs the regular expressions. It may be this that is problematic... |
|
OK, so I think I know what the issue is. The regex is being lazy executed. It is not running until the first read access of one of the properties on the |
|
Maybe this changeset: https://trac.webkit.org/changeset/112454 ? |
In Safari 7 (and other browsers potentially using the latest YARR JIT library) regular expressions are not always executed immediately that they are called. The regex is only evaluated (lazily) when you first access properties on the `matches` result object returned from the regex call. In the case of `decodeEntities()`, we were updating this returned object, `parts[0] = ''`, before accessing it, `if (parts[2])', and so our change was overwritten by the result of executing the regex. The solution here is not to modify the match result object at all. We only need to make use of the three match results directly in code. Developers should be aware, in the future, when using regex, to read from the result object before making modifications to it. There is no additional test committed here, because when run against Safari 7, this bug caused numerous specs to fail, which are all fixed by this commit. Closes angular#5193 Closes angular#5192
In Safari 7 (and other browsers potentially using the latest YARR JIT library) regular expressions are not always executed immediately that they are called. The regex is only evaluated (lazily) when you first access properties on the `matches` result object returned from the regex call. In the case of `decodeEntities()`, we were updating this returned object, `parts[0] = ''`, before accessing it, `if (parts[2])', and so our change was overwritten by the result of executing the regex. The solution here is not to modify the match result object at all. We only need to make use of the three match results directly in code. Developers should be aware, in the future, when using regex, to read from the result object before making modifications to it. There is no additional test committed here, because when run against Safari 7, this bug caused numerous specs to fail, which are all fixed by this commit. Closes angular#5193 Closes angular#5192
Clearing the match before the conditional branch seems to (inexplicably)
break Safari 7 on OSX 10.9, except when stepping through line by line.
Moving the assignment to after the conditional branch corrects this
behaviour.
Closes #5192