Simplify browser regression test for unused timers in Edge #2806#3252
Conversation
…atform#2806 The regression test added for an issue with the Edge implementation running out of handles because of too many timer being instantiated currently creates thousands of browser functions. The actual issue occurs because of too many calls to processOSMessagesUntil(), each creating a timer. This already happens when accessing the WebView instance through the setText() call inside the test. Thus, the creation of browser functions in the test is obsolete, which is why this change removes it. Contributes to eclipse-platform#2806
29d5466 to
4eb6309
Compare
This was known when the test was written - but I included creating both setText and adding functions because the test was written in a more black box way. A future optimization could change how setText is implemented, and IIRC in Linux the setText and new BrowserFunction do behave quite differently, the former simply calling GTK's function and the the latter using async browser execution. What problem were you trying to solve with this change? |
The problem is that the way how the BrowserFunction creation in this test behaves leads to a significant performance degradation of the test with #3166. In fact, the test would not fulfill its purpose anymore when the execution time is too high (as the timers would not accumulate accordingly anymore).
I see, that's a valid point. Then we need to revise the test anyway to capture this information, as it's hardly possible to derive constraints from the test when reading/revising it -- In particular, when there is specific behavior on Linux we want to cover with the test as well, as the original issue was only about Windows, so I didn't expect any Linux specifics to be covered with the test additionally. |
|
OK - it is a regression test for an actual problem on Windows (which your simplification still covers) and was written in a way that could catch similar issues in the future on all platforms. I am ok with this PR. FWIW the original bug (in my customer's code) was primarily about creating thousands of BrowserFunction - the creating of thousands of BrowserFunctions in the customer's code was itself a long standing (unknown) bug in their code that was exposed when changing from IE to Edge. I think that issue is fixed in their code. |
The regression test added for an issue with the Edge implementation running out of handles because of too many timer being instantiated currently creates thousands of browser functions. The actual issue occurs because of too many calls to processOSMessagesUntil(), each creating a timer. This already happens when accessing the WebView instance through the setText() call inside the test. Thus, the creation of browser functions in the test is obsolete, which is why this change removes it.
Contributes to #2806
Executing the adapted test method with reverted #2806 shows that the test stays a proper regression test:
