Skip to content

Simplify browser regression test for unused timers in Edge #2806#3252

Merged
HeikoKlare merged 1 commit intoeclipse-platform:masterfrom
HeikoKlare:simplify-test-2806
Apr 18, 2026
Merged

Simplify browser regression test for unused timers in Edge #2806#3252
HeikoKlare merged 1 commit intoeclipse-platform:masterfrom
HeikoKlare:simplify-test-2806

Conversation

@HeikoKlare
Copy link
Copy Markdown
Contributor

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:
image

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 18, 2026

Test Results

  182 files  ±0    182 suites  ±0   26m 30s ⏱️ + 1m 18s
4 703 tests ±0  4 682 ✅ ±0   21 💤 ±0  0 ❌ ±0 
6 716 runs  ±0  6 561 ✅ ±0  155 💤 ±0  0 ❌ ±0 

Results for commit 4eb6309. ± Comparison against base commit eb769c4.

♻️ This comment has been updated with latest results.

…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
@HeikoKlare HeikoKlare marked this pull request as ready for review April 18, 2026 09:49
@HeikoKlare HeikoKlare merged commit 4897047 into eclipse-platform:master Apr 18, 2026
24 checks passed
@HeikoKlare HeikoKlare deleted the simplify-test-2806 branch April 18, 2026 10:40
@jonahgraham
Copy link
Copy Markdown
Contributor

This already happens when accessing the WebView instance through the setText() call inside the test.

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?

@HeikoKlare
Copy link
Copy Markdown
Contributor Author

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).

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

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.

@jonahgraham
Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants