Remove serialization workarounds for ie 6/7 and rhino (#9578) #9876
Conversation
There was a problem hiding this comment.
Thanks for taking this on, legacy code can be a slog to remove... I will note for posterity that at least up to IE9 (and I think 10?) there is some memory leak that occurs in this code as well, but I can't remember the specifics - but since even IE11 is effectively dead, we should go ahead and ignore this.
It looks like this still leaves the eval() call in the client code, if the server ends up sending version < 8 (and the default is 7), see
I can see an argument for removing this in phases, so that old clients will still work - but if that's the plan, I'd want to consider bumping the SERIALIZATION_STREAM_VERSION to 8, and warning on old versions.
Also: the rhino notes are about the legacy dev mode implementation of gwt-rpc (to avoid needing to emulate all the string operations in JS). It is technically possible that this will break GWTTestCase for htmlunit "hosted mode" tests (hosted mode == legacy dev mode)
To deal with that and still kill this old code, I'd consider removing the linked file above and replacing it with the supersource version (the second link in this reply, except with the @GwtScriptOnly removed, and javadoc updated to indicate that it is no longer supersourced).
What do you think?
| writePayload(stream); | ||
| writeStringTable(stream); | ||
| writeHeader(stream); | ||
| StringBuffer buffer = new StringBuffer(capacityGuess); |
There was a problem hiding this comment.
As long as we're updating this, consider replacing StringBuffer with StringBuilder for better single-threaded performance? From https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StringBuffer.html
As of release JDK 5, this class has been supplemented with an equivalent class designed for use by a single thread, StringBuilder. The StringBuilder class should generally be used in preference to this one, as it supports all of the same operations but it is faster, as it performs no synchronization.
There was a problem hiding this comment.
I updated to use StringBuilder instead of StringBuffer and changed the default serialization format to JSON.
I'm not sure how best to implement the rest of your suggestions though. But they seem logical.
| : Integer.MAX_VALUE; | ||
|
|
||
| while (i < length && charVector.getSize() < maxSegmentVectorSize) { | ||
| while (i < length) { |
There was a problem hiding this comment.
This can be removed. The same loop definition is in the line above.
There was a problem hiding this comment.
Removed the extra loop
|
Any chance to get this PR in the next release (because this enabled a CSP w/o "unsafe-eval")? |
|
Anyone who wants to pick this up and continue it, removing other related dead code, would be welcome to do so. I don't presently have a plan to take this on, so we're looking for a contributor or sponsor to focus on it. See notes in my review above for other steps that should be taken to correct this more than superficially. |
|
Would it be possible to just merge this now and make a separate issue for the dead code removal? The current code fails with any site that uses CSP under the specific conditions of the issue as |
|
@codemasterover9000 to confirm, the work you're doing here mostly checks two boxes so far:
Unless I'm mistaken, you could just configure your own project to use version 8, and avoid the codepaths that you are removing in this PR? That would solve this except for too-big payloads (strings longer than MAX_STRING_NODE_LENGTH, - a much smaller PR could update that with no risk of being half finished, and just make that constant able to be configured by another system property, so you could set the limit as high as you wanted (Integer.MAX_VALUE for example). I'm guessing you haven't run all tests on this PR, but looking briefly at some dev mode tests (which use rhino in legacy dev mode) it appears that some tests may fail from this change. Notably, here's the test that validates that Rhino still has the bug (and so this PR will break it): Running the I show one failure: ValueTypesTest.testString_over64KBWithUnicode() That does not fail before your changes. I have not yet run the full set of tests, was waiting for the branch to be more completed to try to proceed. What do you think of trying to limit the scope of the patch as I proposed above, so that it can be disabled on a per-project basis, until we manage these remaining issues and complete the related work? |
|
Yes we use version 8. I agree that making the threshold a system property as you suggested would be a better option for a less impactful change. I made a new pull request here #9961 |
Removed the workarounds for IE6/7 issues and rhino. IE 6/7 are not relevant anymore today. Not sure if this is the rhino issue referenced in the code: https://bugzilla.mozilla.org/show_bug.cgi?id=179068. But it seems to have been fixed a long time ago.
Fixes #9578