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.

Sign up for GitHub

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

Jump to bottom

Ensure winit windows are cleaned up before TLS disappears. #29424

Merged
merged 1 commit into from
Feb 27, 2023
Merged

Ensure winit windows are cleaned up before TLS disappears. #29424

merged 1 commit into from
Feb 27, 2023

Conversation

jdm
Copy link
Member

@jdm jdm commented Feb 27, 2023

Since the winit update, closing Servo on macOS can lead to a crash. This is because winit panics while trying to clean up the window, since it requires the TLS to be available. Right now this code doesn't run until Servo's TLS is being destroyed. There's no reason to use a thread local value to store the window information, however, and moving it into the App object avoids the panic.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because we don't run non-headless tests on macOS.

@jdm jdm requested a review from mrobinson February 27, 2023 00:22
@mrobinson
Copy link
Member

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 1b99dfd has been approved by mrobinson

@bors-servo
Copy link
Contributor

⌛ Testing commit 1b99dfd with merge 2439eab...

bors-servo added a commit that referenced this pull request Feb 27, 2023
@bors-servo
Auto merge of #29424 - jdm:macos-shutdown-crash, r=mrobinson
2439eab 
Ensure winit windows are cleaned up before TLS disappears.

Since the winit update, closing Servo on macOS can lead to a crash. This is because winit panics while trying to clean up the window, since it requires the TLS to be available. Right now this code doesn't run until Servo's TLS is being destroyed. There's no reason to use a thread local value to store the window information, however, and moving it into the App object avoids the panic.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because we don't run non-headless tests on macOS.
@mrobinson
Copy link
Member

This seems like a good fix and a nice cleanup.

@bors-servo
Copy link
Contributor

💔 Test failed - checks-github

@CYBAI
Copy link
Member

CYBAI commented Feb 27, 2023

build failure on windows, linux and macos 👀

error[E0614]: type `std::collections::HashMap<WindowId, Rc<(dyn WindowPortsMethods + 'static)>>` cannot be dereferenced
   --> ports/winit/app.rs:196:35
    |
196 |         for (_win_id, window) in &*self.windows {
    |                                   ^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0614`.

@jdm
Copy link
Member Author

jdm commented Feb 27, 2023

@bors-servo r+
Missed committing a local change.

@bors-servo
Copy link
Contributor

📌 Commit a99dd55 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit a99dd55 with merge 3b807fc...

@github-actions
Copy link

Results from try job (#4283851934):

Flaky unexpected result (18)
  • TIMEOUT [expected OK] /FileAPI/url/url-charset.window.html (#26997)
    • TIMEOUT [expected PASS] subtest: Blob charset should override any auto-detected charset.
  • TIMEOUT [expected OK] /_webgl/conformance/uniforms/out-of-bounds-uniform-array-access.html (#26225)
    • NOTRUN [expected PASS] subtest: Overall test
  • OK /css/CSS2/floats/hit-test-floats-002.html (#25733)
    • FAIL [expected PASS] subtest: Hit test float
  • OK /css/CSS2/normal-flow/block-in-inline-hittest-002.html (#29057)
    • FAIL [expected PASS] subtest: elementFromPoint
  • OK /css/CSS2/normal-flow/block-in-inline-hittest-relpos-zindex.html (#29052)
    • FAIL [expected PASS] subtest: block-in-inline-hittest-relpos-zindex
  • OK /css/CSS2/normal-flow/hit-test-anonymous-block.html (#25807)
    • FAIL [expected PASS] subtest: Hit test beside line of text inside anonymous block
  • OK /css/css-transitions/properties-value-inherit-002.html (#21486)
    • FAIL [expected PASS] subtest: color color(rgba) / values
    • FAIL [expected PASS] subtest: font-size length(pt) / values
    • FAIL [expected PASS] subtest: font-size length(pc) / values
    • FAIL [expected PASS] subtest: font-size length(px) / values
    • FAIL [expected PASS] subtest: font-size length(em) / values
    • FAIL [expected PASS] subtest: font-size length(ex) / values
    • FAIL [expected PASS] subtest: font-size length(mm) / values
    • FAIL [expected PASS] subtest: font-size length(cm) / values
    • FAIL [expected PASS] subtest: font-size length(in) / values
    • FAIL [expected PASS] subtest: font-size percentage(%) / values
    • And 40 more unexpected results...
  • OK /css/cssom-view/CaretPosition-001.html (#21338)
    • FAIL [expected PASS] subtest: Element at (400, 100)
  • OK /css/cssom-view/elementsFromPoint-iframes.html (#19273)
    • PASS [expected FAIL] subtest: elementsFromPoint on inner documents
  • TIMEOUT [expected ERROR] /html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html (#28541)
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin.window.html (#29049)
    • FAIL [expected PASS] subtest: Same-origin navigation started from unload handler must be ignored
  • OK /html/browsers/history/the-history-interface/traverse_the_history_5.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html (#22667)
    • TIMEOUT [expected FAIL] subtest: Check that popups from a sandboxed iframe escape the sandbox if
      allow-popups-to-escape-sandbox is used
  • CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html (#24057)
  • OK /html/semantics/embedded-content/the-img-element/non-active-document.html (#21544)
    • PASS [expected FAIL] subtest: createHTMLDocument
    • PASS [expected FAIL] subtest: <template>
  • TIMEOUT [expected OK] /html/webappapis/scripting/processing-model-2/integration-with-the-javascript-job-queue/promise-job-entry-different-function-realm.html (#25805)
    • TIMEOUT [expected FAIL] subtest: Fulfillment handler on pending-then-fulfilled promise
    • TIMEOUT [expected FAIL] subtest: Rejection handler on pending-then-rejected promise
  • TIMEOUT [expected OK] /html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.html (#26371)
    • TIMEOUT [expected FAIL] subtest: delayed handling: delaying handling rejected promise created from createImageBitmap will cause both events to fire
  • TIMEOUT [expected OK] /wasm/jsapi/functions/entry.html (#27087)
    • TIMEOUT [expected FAIL] subtest: Start function
Stable unexpected results that are known to be intermittent (22)
  • OK /_mozilla/mozilla/scrollBy.html (#21321)
    • FAIL [expected PASS] subtest: Ensure that the window.scrollBy function affects scroll position as expected
  • OK /css/CSS2/floats/hit-test-floats-001.html (#23693)
    • PASS [expected FAIL] subtest: hit-test-floats-001
  • OK /css/CSS2/floats/hit-test-floats-004.html (#25804)
    • FAIL [expected PASS] subtest: Miss float below something else
  • FAIL [expected TIMEOUT] /css/CSS2/selectors/first-letter-punctuation-087.xht (#28827)
  • OK /css/css-flexbox/hittest-anonymous-box.html (#27091)
    • FAIL [expected PASS] subtest: Hit-testing within an anonymous flex-item should return the flexbox as the hittest result.
  • OK /css/css-transitions/properties-value-inherit-003.html (#26435)
    • PASS [expected FAIL] subtest: border-top-width length-em(em) / events
    • PASS [expected FAIL] subtest: border-right-width length-em(em) / events
    • PASS [expected FAIL] subtest: border-bottom-width length-em(em) / events
    • PASS [expected FAIL] subtest: border-left-width length-em(em) / events
    • PASS [expected FAIL] subtest: padding-bottom length-em(em) / events
    • PASS [expected FAIL] subtest: padding-left length-em(em) / events
    • PASS [expected FAIL] subtest: padding-right length-em(em) / events
    • PASS [expected FAIL] subtest: padding-top length-em(em) / events
    • PASS [expected FAIL] subtest: margin-bottom length-em(em) / events
    • PASS [expected FAIL] subtest: margin-left length-em(em) / events
    • And 19 more unexpected results...
  • OK /css/cssom-view/elementFromPoint-list-001.html (#23915)
    • FAIL [expected PASS] subtest: <li>Inside 1</li>
    • FAIL [expected PASS] subtest: <li>Inside 3</li>
    • FAIL [expected PASS] subtest: <li>Image Inside 1</li>
    • FAIL [expected PASS] subtest: <li>Image Inside 2</li>
  • TIMEOUT /fetch/metadata/generated/css-images.sub.tentative.html (#29047)
    • PASS [expected TIMEOUT] subtest: background-image sec-fetch-user - Not sent to non-trustworthy same-origin destination
    • TIMEOUT [expected PASS] subtest: background-image sec-fetch-site - HTTPS downgrade (header not sent)
  • OK /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-return-value-handling-dynamic.html (#28066)
    • FAIL [expected PASS] subtest: D83D DE0D set in href="" targeting a frame and clicked
  • OK /html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html (#23849)
    • FAIL [expected PASS] subtest: The document for a standalone media file should have one child in the body.
  • OK /html/browsers/the-window-object/open-close/creating_browsing_context_test_01.html (#29046)
    • PASS [expected FAIL] subtest: first argument: absolute url
  • OK [expected TIMEOUT] /html/interaction/focus/the-autofocus-attribute/update-the-rendering.html (#24145)
    • FAIL [expected TIMEOUT] subtest: "Flush autofocus candidates" should be happen before a scroll event and animation frame callbacks
  • OK [expected CRASH] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
    • FAIL [expected NOTRUN] subtest: Check that popups from a sandboxed iframe do not escape the sandbox
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html (#22154)
    • FAIL [expected NOTRUN] subtest: Check that popups from a sandboxed iframe do not escape the sandbox
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html (#24066)
    • FAIL [expected NOTRUN] subtest: Check that popups from a sandboxed iframe do not escape the sandbox
  • OK /html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html (#23205)
    • PASS [expected FAIL] subtest: Check that rel=noopener with target=_self does a normal load
  • OK /html/webappapis/dynamic-markup-insertion/document-write/module-delayed.html (#27659)
    • PASS [expected FAIL] subtest: async document.write in a module
  • OK /html/webappapis/dynamic-markup-insertion/document-write/module-tla-delayed.html (#29137)
    • PASS [expected FAIL] subtest: document.write in an imported module
  • TIMEOUT /html/webappapis/scripting/events/compile-event-handler-settings-objects.html (#24246)
    • FAIL [expected TIMEOUT] subtest: The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document
  • OK [expected TIMEOUT] /html/webappapis/scripting/processing-model-2/integration-with-the-javascript-job-queue/promise-job-entry.html (#25805)
    • FAIL [expected TIMEOUT] subtest: Fulfillment handler on pending-then-fulfilled promise
    • FAIL [expected TIMEOUT] subtest: Rejection handler on pending-then-rejected promise
  • TIMEOUT [expected CRASH] /webmessaging/broadcastchannel/cross-partition.https.tentative.html (#29058)

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-github
Approved by: jdm
Pushing 3b807fc to master...

@bors-servo bors-servo merged commit 3b807fc into servo:master Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrobinson mrobinson mrobinson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jdm @mrobinson @bors-servo @CYBAI