1897088 - structs that are uploaded should be `repr(C)` and implement `bytemuck::Pod`

Status: RESOLVED WONTFIX

(Core :: Graphics: WebRender, task, P3)

Description

[Nicolas Silva [:nical]]

Structures that we pass to OpenGL directly via https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/device/gl.rs#3513 need to uphold a few guarantees like repr(C) and that there is no padding in the data, otherwise it's UB.

Comment 1

[Mike Hommey [:glandium]]

Of all the things that end up on the function call you pointed out, only LineDecorationJob is missing a repr(C).

As for bytemuck::Pod, it has two caveats:

• it requires Copy
• for everything that uses types from external crates, like Box2D, it needs to be implemented manually, which removes the advantage of the checks the derive macro does. Still better than nothing, though.

I guess the big question is whether all the affected types and all the types they contain indirectly are fine to be Copy.

List of the affected types: CopyInstance, BlurInstance, ScalingInstance, SvgFilterInstance, SVGFEFilterInstance, BorderInstance, ClipMaskInstanceRect, ClipMaskInstanceBoxShadow, PrimitiveInstanceData, CompositeInstance, ClearInstance, MaskInstance, ConicGradientInstance, FastLinearGradientInstance, LinearGradientInstance, RadialGradientInstance, LineDecorationJob, DebugFontVertex, DebugColorVertex, Box2D, Size2D, RenderTaskAddress, BlurDirection, GpuCacheAddress, Point2D, PremultipliedColorF, ClipMaskInstanceCommon, ClipData, BoxShadowData, TexelRect, CompositorTransform, TransformPaletteId, ClipSpace, Vector2D, ColorU. (but note that I haven't dug deeper than one level, so far)

Comment 2

[Mike Hommey [:glandium]]

mmmm, BlurDirection is an enum with 2 possible values. It's not Pod. Likewise for ClipSpace.

Comment 3

[Mike Hommey [:glandium]]

Oh, Box2D, and other types coming from euclid come with a bytemuck::Pod impl behind a feature.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1897088 - Add a bytemuck::Pod bound to Device::update_vbo_data.

This unveiled a couple UB-ish uses (missing repr(C) or use of enums).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6d6086ce57f
Add a bytemuck::Pod bound to Device::update_vbo_data. r=gfx-reviewers,supply-chain-reviewers,nical

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f6d6086ce57f

Comment 7

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/8a43e4098e24

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8cef704b8dd
Add a bytemuck::Pod bound to Device::update_vbo_data. r=gfx-reviewers,supply-chain-reviewers,nical

Comment 9

[tszentpeteri]

Backed out as requested by glandium for causing test-linux1804-64-asan-qr/opt-mochitest-remote bustage.

• Backout link
• Push with failures
• Failure Log link
• Failure log:

[task 2024-05-23T06:09:57.927Z] 06:09:57     INFO - TEST-START | remote/shared/test/browser/browser_NavigationManager_no_navigation.js
[task 2024-05-23T06:09:59.719Z] 06:09:59     INFO - GECKO(11290) | 1716444599717	RemoteAgent	TRACE	[34] NavigationListener onLocationChange, location: https://example.com/document-builder.sjs?html=test
[task 2024-05-23T06:09:59.991Z] 06:09:59     INFO - GECKO(11290) | 1716444599990	RemoteAgent	TRACE	[34] NavigationListener onStateChange, stateFlags: 983041, status: 0, isStart: true, isStop: false, isNetwork: true, isBindingAborted: false, targetURI: https://example.com/document-builder.sjs?html=test
[task 2024-05-23T06:10:00.002Z] 06:10:00     INFO - GECKO(11290) | 1716444600001	RemoteAgent	TRACE	[9822135f-2ed0-4362-980b-6b973048fc7e] Navigation started for url: https://example.com/document-builder.sjs?html=test (90045d8e-1b99-452a-a612-d0b66037e897)
[task 2024-05-23T06:10:00.239Z] 06:10:00     INFO - GECKO(11290) | 1716444600238	RemoteAgent	TRACE	[34] NavigationListener onStateChange, stateFlags: 196610, status: 0, isStart: false, isStop: false, isNetwork: false, isBindingAborted: false, targetURI: https://example.com/document-builder.sjs?html=test
[task 2024-05-23T06:10:00.332Z] 06:10:00     INFO - GECKO(11290) | 1716444600330	RemoteAgent	TRACE	[34] NavigationListener onStateChange, stateFlags: 196612, status: 0, isStart: false, isStop: false, isNetwork: false, isBindingAborted: false, targetURI: https://example.com/document-builder.sjs?html=test
[task 2024-05-23T06:10:00.341Z] 06:10:00     INFO - GECKO(11290) | 1716444600331	RemoteAgent	TRACE	[34] NavigationListener onStateChange, stateFlags: 196612, status: 0, isStart: false, isStop: false, isNetwork: false, isBindingAborted: false, targetURI: https://example.com/document-builder.sjs?html=test
[task 2024-05-23T06:10:00.453Z] 06:10:00     INFO - GECKO(11290) | 1716444600452	RemoteAgent	TRACE	[34] NavigationListener onStateChange, stateFlags: 131088, status: 0, isStart: false, isStop: true, isNetwork: false, isBindingAborted: false, targetURI: https://example.com/document-builder.sjs?html=test
[task 2024-05-23T06:10:00.477Z] 06:10:00     INFO - GECKO(11290) | 1716444600476	RemoteAgent	TRACE	[9822135f-2ed0-4362-980b-6b973048fc7e] Navigation finished for url: https://example.com/document-builder.sjs?html=test (90045d8e-1b99-452a-a612-d0b66037e897)
[task 2024-05-23T06:10:51.413Z] 06:10:51     INFO - GECKO(11290) | LLVM ERROR: out of memory
[task 2024-05-23T06:10:51.413Z] 06:10:51     INFO - GECKO(11290) | Allocation failed
[task 2024-05-23T06:10:51.413Z] 06:10:51    ERROR - GECKO(11290) | ==11368==ERROR: AddressSanitizer: out of memory: failed to allocate 0xe000 (57344) bytes of Create (error code: 12)
[task 2024-05-23T06:10:51.414Z] 06:10:51     INFO - GECKO(11290) | ERROR: Failed to mmap
[task 2024-05-23T06:10:51.414Z] 06:10:51     INFO - GECKO(11290) | XPCOMGlueLoad error for file /builds/worker/workspace/build/application/firefox/libmozgtk.so:
[task 2024-05-23T06:10:51.414Z] 06:10:51     INFO - GECKO(11290) | libgcrypt.so.20: failed to map segment from shared object
[task 2024-05-23T06:10:51.581Z] 06:10:51     INFO - GECKO(11290) | PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
[task 2024-05-23T06:10:51.581Z] 06:10:51     INFO - GECKO(11290) | Stack dump:
[task 2024-05-23T06:10:51.718Z] 06:10:51     INFO - GECKO(11290) | 0.	Program arguments: /builds/worker/workspace/build/application/firefox/llvm-symbolizer --demangle --inlines --default-arch=x86_64
[task 2024-05-23T06:17:46.662Z] 06:17:46     INFO - GECKO(11290) | Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
[task 2024-05-23T06:17:47.189Z] 06:17:47     INFO - Buffered messages logged at 06:09:57
[task 2024-05-23T06:17:47.189Z] 06:17:47     INFO - Entering test bound testDocumentOpenWriteClose
[task 2024-05-23T06:17:47.190Z] 06:17:47     INFO - Buffered messages logged at 06:09:58
[task 2024-05-23T06:17:47.192Z] 06:17:47     INFO - Console message: [JavaScript Warning: "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”." {file: "https://example.com/document-builder.sjs?html=test" line: 0}]
[task 2024-05-23T06:17:47.192Z] 06:17:47     INFO - Buffered messages logged at 06:09:59
[task 2024-05-23T06:17:47.192Z] 06:17:47     INFO - TEST-PASS | remote/shared/test/browser/browser_NavigationManager_no_navigation.js | No event recorded - 
[task 2024-05-23T06:17:47.192Z] 06:17:47     INFO - Replace the document
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - Console message: [JavaScript Warning: "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”." {file: "eval" line: 3}]
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - TEST-PASS | remote/shared/test/browser/browser_NavigationManager_no_navigation.js | No event recorded after replacing the document - 
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - Reload the page, which should trigger a navigation
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - Buffered messages logged at 06:10:00
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - Console message: [JavaScript Warning: "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”." {file: "https://example.com/document-builder.sjs?html=test" line: 0}]
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - TEST-PASS | remote/shared/test/browser/browser_NavigationManager_no_navigation.js | Recorded navigation events - 
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - Leaving test bound testDocumentOpenWriteClose
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - Buffered messages finished
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - TEST-UNEXPECTED-TIMEOUT | remote/shared/test/browser/browser_NavigationManager_no_navigation.js | application timed out after 370 seconds with no output
[task 2024-05-23T06:17:47.193Z] 06:17:47     INFO - TEST-INFO took 468079ms
[task 2024-05-23T06:17:47.478Z] 06:17:47     INFO - Buffered messages finished
[task 2024-05-23T06:17:47.478Z] 06:17:47  WARNING - Force-terminating active process(es).
[task 2024-05-23T06:17:47.478Z] 06:17:47     INFO - Determining child pids from psutil...
[task 2024-05-23T06:17:47.627Z] 06:17:47     INFO - [11368, 11396, 11441, 11449, 11451, 11469, 11492, 11555, 11571, 11603, 11604, 11607, 11663, 11690, 11712, 11755, 11757, 11804, 11805, 11860]
[task 2024-05-23T06:17:48.890Z] 06:17:48     INFO - ==> process 11290 launched child process 11368
[task 2024-05-23T06:17:48.890Z] 06:17:48     INFO - ==> process 11290 launched child process 11396
[task 2024-05-23T06:17:48.891Z] 06:17:48     INFO - ==> process 11290 launched child process 11441
<...>
[task 2024-05-23T06:19:38.940Z] 06:19:38     INFO - GECKO(12348) | console.error: ({})
[task 2024-05-23T06:19:42.542Z] 06:19:42     INFO - GECKO(12348) | [ERROR error_support::handling] suggest-unexpected: Error from Remote Settings: Error parsing URL: relative URL with a cannot-be-a-base base
[task 2024-05-23T06:19:42.559Z] 06:19:42     INFO - GECKO(12348) | console.error: URLBar - QuickSuggest.SuggestBackendRust: "Ingest error: Error from Remote Settings: Error parsing URL: relative URL with a cannot-be-a-base base"
[task 2024-05-23T06:19:42.906Z] 06:19:42     INFO - GECKO(12348) | 1716445182905	Marionette	TRACE	Received observer notification browser-idle-startup-tasks-finished
[task 2024-05-23T06:19:42.941Z] 06:19:42     INFO - GECKO(12348) | 1716445182940	RemoteAgent	TRACE	[9] ProgressListener Start: expectNavigation=false resolveWhenStarted=false unloadTimeout=40000 waitForExplicitStart=false
[task 2024-05-23T06:19:42.944Z] 06:19:42     INFO - GECKO(12348) | 1716445182941	RemoteAgent	TRACE	[9] ProgressListener Setting unload timer (40000ms)
[task 2024-05-23T06:19:42.944Z] 06:19:42     INFO - GECKO(12348) | 1716445182941	RemoteAgent	TRACE	[9] Wait for initial navigation: isInitial=false, isLoadingDocument=false
[task 2024-05-23T06:19:42.945Z] 06:19:42     INFO - GECKO(12348) | 1716445182942	RemoteAgent	TRACE	[9] Document already finished loading: about:blank
[task 2024-05-23T06:19:42.945Z] 06:19:42     INFO - GECKO(12348) | 1716445182943	RemoteAgent	TRACE	[9] ProgressListener Stop: has error=false url=about:blank
[task 2024-05-23T06:19:42.993Z] 06:19:42     INFO - GECKO(12348) | 1716445182991	Marionette	DEBUG	3 <- [1,1,null,{"sessionId":"ad54992c-35d4-4e75-96ef-c7332788d652","capabilities":{"browserName":"firefox","browserVersion":"128.0a1","platformName":"linux","acceptInsecureCerts":false,"pageLoadStrategy":"normal","setWindowRect":true,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000},"strictFileInteractability":true,"unhandledPromptBehavior":"dismiss and notify","userAgent":"Mozilla/5.0 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0","moz:accessibilityChecks":false,"moz:buildID":"20240523043730","moz:headless":false,"moz:platformVersion":"4.4.0-1014-aws","moz:processID":12348,"moz:profile":"/tmp/tmp9kjme047.mozrunner","moz:shutdownTimeout":300000,"moz:webdriverClick":true,"moz:windowless":false,"proxy":{}}}]
[task 2024-05-23T06:19:43.078Z] 06:19:43     INFO - GECKO(12348) | 1716445183077	Marionette	DEBUG	3 -> [0,2,"Addon:Install",{"path":"/tmp/tmpj67glkob.zip","temporary":false}]
[task 2024-05-23T06:19:43.465Z] 06:19:43     INFO - GECKO(12348) | 1716445183464	Marionette	DEBUG	3 <- [1,2,null,{"value":"[e-mail]"}]
[task 2024-05-23T06:19:43.558Z] 06:19:43     INFO - GECKO(12348) | 1716445183555	Marionette	DEBUG	3 -> [0,3,"Addon:Install",{"path":"/tmp/tmp5f1zc0fb.zip","temporary":false}]
[task 2024-05-23T06:19:43.665Z] 06:19:43     INFO - GECKO(12348) | 1716445183664	Marionette	DEBUG	3 <- [1,3,null,{"value":"[e-mail]"}]
[task 2024-05-23T06:19:43.699Z] 06:19:43     INFO - GECKO(12348) | 1716445183698	Marionette	DEBUG	3 -> [0,4,"Marionette:GetContext",{}]
[task 2024-05-23T06:19:43.700Z] 06:19:43     INFO - GECKO(12348) | 1716445183698	Marionette	DEBUG	3 <- [1,4,null,{"value":"content"}]
[task 2024-05-23T06:19:43.702Z] 06:19:43     INFO - GECKO(12348) | 1716445183701	Marionette	DEBUG	3 -> [0,5,"Marionette:SetContext",{"value":"chrome"}]
[task 2024-05-23T06:19:43.703Z] 06:19:43     INFO - GECKO(12348) | 1716445183701	Marionette	DEBUG	3 <- [1,5,null,{"value":null}]
[task 2024-05-23T06:19:43.728Z] 06:19:43     INFO - GECKO(12348) | 1716445183727	Marionette	DEBUG	3 -> [0,6,"WebDriver:ExecuteScript",{"script":"/* This Source Code Form is subject to the terms of the Mozilla Public\n * License, v. 2.0. If a copy of the MPL was not distr ... s which flavor and url to load.\nlet ev = new CustomEvent(\"mochitest-load\", { detail: [flavor, url] });\nwin.dispatchEvent(ev);","args":[{"flavor":"browser-chrome","testUrl":"about:blank"}],"newSandbox":true,"sandbox":"default","line":2167,"filename":"tests/mochitest/runtests.py"}]
[task 2024-05-23T06:19:43.740Z] 06:19:43     INFO - GECKO(12348) | 1716445183739	RemoteAgent	TRACE	WebDriverProcessData actor created for PID 12348
[task 2024-05-23T06:19:43.743Z] 06:19:43     INFO - GECKO(12348) | 1716445183742	Marionette	TRACE	[1] MarionetteCommands actor created for window id 2
[task 2024-05-23T06:19:43.791Z] 06:19:43     INFO - GECKO(12348) | 1716445183790	RemoteAgent	TRACE	Received observer notification domwindowopened
[task 2024-05-23T06:19:43.806Z] 06:19:43     INFO - GECKO(12348) | 1716445183805	Marionette	DEBUG	3 <- [1,6,null,{"value":null}]
[task 2024-05-23T06:19:43.847Z] 06:19:43     INFO - GECKO(12348) | 1716445183846	Marionette	DEBUG	3 -> [0,7,"Marionette:SetContext",{"value":"content"}]
[task 2024-05-23T06:19:43.850Z] 06:19:43     INFO - GECKO(12348) | 1716445183847	Marionette	DEBUG	3 <- [1,7,null,{"value":null}]
[task 2024-05-23T06:19:43.938Z] 06:19:43     INFO - GECKO(12348) | 1716445183937	Marionette	DEBUG	3 -> [0,8,"WebDriver:DeleteSession",{}]
[task 2024-05-23T06:19:43.944Z] 06:19:43     INFO - GECKO(12348) | 1716445183939	Marionette	TRACE	[1] MarionetteCommands actor destroyed for window id 2
[task 2024-05-23T06:19:43.982Z] 06:19:43     INFO - GECKO(12348) | 1716445183981	Marionette	DEBUG	3 <- [1,8,null,{"value":null}]
[task 2024-05-23T06:19:43.985Z] 06:19:43     INFO - runtests.py | Waiting for browser...
[task 2024-05-23T06:19:43.993Z] 06:19:43     INFO - GECKO(12348) | 1716445183985	Marionette	DEBUG	Closed connection 3
[task 2024-05-23T06:19:44.969Z] 06:19:44     INFO - *** Start BrowserChrome Test Results ***
[task 2024-05-23T06:19:45.081Z] 06:19:45     INFO - checking window state
[task 2024-05-23T06:19:45.235Z] 06:19:45     INFO - TEST-START | remote/webdriver-bidi/test/browser/browser_RemoteValue.js

Comment 10

[Mike Hommey [:glandium]]

This is quite baffling... enabling the bytemuck feature of euclid is what triggers whatever this is.

Comment 11

[Mike Hommey [:glandium]]

What do you think of making it optional and only built on e.g. wrench builds? That would catch UB, while avoiding whatever it is that is happening, as well as avoiding the extra derive(Copy) on many types.

Comment 12

[Nicolas Silva [:nical]]

Wow!

On second thought, I suspect that not using Pod, while a bit brittle, is actually fine given the way we currently use these types. However the missing #[repr(C)] is important and addresses real UB, so we can at least extract that part and land it.

About optionally requiring Pod, I suspect that it's going to cause more friction than its worth but I don't feel very strongly, I'll ask around.

Comment 13

[Mike Hommey [:glandium]]

so we can at least extract that part and land it.

Already did in bug 1898614

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:glandium, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Nicolas Silva [:nical] from comment #12)

About optionally requiring Pod, I suspect that it's going to cause more friction than its worth but I don't feel very strongly, I'll ask around.

Should this be WONTFIX at this point?

Comment 16

[Nicolas Silva [:nical]]

Yeah, let's move on. Maybe we'll try again later and whatever caused the mysterious asan issue won't happen then. In the mean time it's not important enough for us to spend to time figuring it out.