Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't register a pending TextLayer until render is invoked (PR 18104 follow-up) #18171

Merged
merged 1 commit into from
May 28, 2024

Conversation

Snuffleupagus
Copy link
Collaborator

After the re-factoring in PR #18104 there's now a theoretical risk that a pending TextLayer is never removed, which we can avoid by not registering it until render is invoked.
Note that this doesn't affect the viewer or tests, but if a third-party user calls new TextLayer(...) without a following call of either the render- or cancel-method we'd block global clean-up without this patch.

@Snuffleupagus Snuffleupagus added text-selection release-blocker Blocker for the upcoming release labels May 26, 2024
@Snuffleupagus Snuffleupagus changed the title Don't register a pending TextLayer until ´render` is invoked (PR 18104 follow-up) Don't register a pending TextLayer until render is invoked (PR 18104 follow-up) May 26, 2024
…104 follow-up)

After the re-factoring in PR 18104 there's now a *theoretical* risk that a pending `TextLayer` is never removed, which we can avoid by not registering it until `render` is invoked.
Note that this doesn't affect the viewer or tests, but if a third-party user calls `new TextLayer(...)` without a following call of either the `render`- or `cancel`-method we'd block global clean-up without this patch.
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/5aff14008bf746c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/a2ca55e415ef05e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/a2ca55e415ef05e/output.txt

Total script time: 3.08 mins

  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.193.163.58:8877/a2ca55e415ef05e/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/5aff14008bf746c/output.txt

Total script time: 27.87 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 21
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/5aff14008bf746c/reftest-analyzer.html#web=eq.log

@timvandermeij
Copy link
Contributor

/botio-windows test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 1

Live output at: http://54.193.163.58:8877/8d1684a05e080be/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/8d1684a05e080be/output.txt

Total script time: 42.80 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 4

Image differences available at: http://54.193.163.58:8877/8d1684a05e080be/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit ee54593 into mozilla:master May 28, 2024
9 checks passed
@timvandermeij
Copy link
Contributor

Thank you for spotting this!

@Snuffleupagus Snuffleupagus deleted the move-pendingTextLayers branch May 28, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocker for the upcoming release text-selection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants