1777535 - Buffer validation

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

Our validation (and ability to not crash when errors occur) isn't quite there yet.

Some notes because the spec is rather confusing:

The spec mentions a few buffer members like [[state]], [[mapping_ranges]], [[mapping]], etc. In general they need to live in the gpu process (the device timeline) and be validated there. Some of them however have to live in the content process, for example [[mapping]], and some of them have to be duplicated, for example [[state]] and [[mappting_range]] are used for validation both on the timeline but also on the content timeline to ensure that getMappedRange does the right thing.

Kangz noted that it was originally designed that way to enfore a strict ordering of the errors, but that there are talks of relaxing the ordering in the future (in which case some state might be validated in the content timeline if it's more convenient), but that's not decided and not what spec says at the moment.

Comment 1

[Nicolas Silva [:nical]]

Dawn code for reference: https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/dawn/wire/client/Buffer.h;

Comment 2

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Return pointer and length in wgpu_server_buffer_get_mapped_range. r=jimb

Also return an empty slice instead of crashing in case of errors.
This is a small piece split off of the buffer mapping rework. Most of the call sites are completely rewritten in followup patches, and error reporting will also come in followups.

Depends on D151374

Comment 3

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Use unsafe shmems. r=jimb

This commit makes WebGPU buffers use unsafe shmems.
Instead of relying on moving the shmem back and forth between the two processes to ensure thread safety, we instead rely on the validation done on both sides. The upside is that it makes it much easier to implement said validation correctly.

In the interest of splitting the buffer mapping rework into small-ish commits, this one puts some structural pieces in place but doesn't necessarily do justice to the final implementation:

• The validation itself is coming in subsequent patches in this series.
• Mapping sub-ranges of the buffer was somewhat implemented in some parts of the parent code and not in others, and was fairly broken as a whole. This commit always maps the entire buffer and proper logic for sub-ranges is coming in another commit.

The main things this commit does put in place:

• Each mappable buffer is associated with a Shmem that is accessible to both sides.
• On the child side, if a buffer is not mappable, then Buffer::mShmem is in its default state (it doesn't point to any shared memory segment).
• On the parent side, if a buffer is not mappable it does not have an entry in mSharedMemoryMap.
• The shmem is always created by the child and destroyed by the parent.

Depends on D151615

Comment 4

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Move the buffer creation code in Buffer.cpp. r=jimb

No funcitonal change here, I like to have the code maintaining and depending on the same invariants to be in the same place.

Depends on D151616

Comment 5

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Map only the required buffer range on the Parent side. r=jimb

Depends on D151617

Comment 6

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Track the buffer mapAsync promise. r=jimb

Per spec (and discussion with someone on the chromium side where spec is vague), the correct behavior should be:

• MapAsync validation happens on the device timeline, so we should reject the promise in mapAsync on the content side if we run into an internal error not described by the spec.
• Unmap immediately rejects all pending mapping promises on the content side (there can be multiple of them since we have to catch that error on the device timeline).

This patch keeps track of pending map promises in an array in a way similar to chromium.
There was some confusion around the the use of uint64_t and size_t which probably originated at point where this code was working differently. This patch uses uint64_t (=BufferAddress) more consistently removing the need for some of the casting and overflow checks.
One notable change in the overall logic is that SetMapped is now called when the buffer is actually in the mapped state (before this patch it was called as soon as the buffer had a pending map request).

Depends on D151618

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Validate getMappedRange against the actually mapped range. r=jimb

Depends on D151619

Comment 8

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Unmap the buffer in Destroy. r=jimb

Per spec, if a buffer is mapped in destory(), unmap() must be called.

Depends on D151620

Comment 9

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Simplify Buffer::Cleanup. r=jimb

Make sure to always clean up any potential content-side state and only avoid sending Destroy each time.

Depends on D151621

Comment 10

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Remove Buffer::Mappable(). r=jimb

It is not used anywhere.

Depends on D151629

Comment 11

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - use Buffer::GetDevice instead of mParent. r=jimb

Another cosmetic change. I've dabbled with IPDL actors too much to not think about WebGPUParent when reading "mParent". Also the parent-child relationship between Device and Buffer is not very obvious to me (nor is it part of the specification).
So I find that wrapping mParent in a GetDevice method to make the code easier to understand. It also makes it explicit that the parent pointer cannot be null.

Depends on D151630

Comment 12

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Move the remaining buffer logic in Device.cpp into Buffer.cpp. r=jimb

Having the code in the same place makes it easier to follow. This made me realize that the validation of aMode in mapAsync has to move to the device side (fix coming in a followup).

Depends on D151631

Comment 13

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Validate mapAync mode on the parent side. r=jimb

Depends on D151632

Comment 14

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Don't crash when buffer_map_async returns an error. r=jimb

The error is handled in the callbacks.

Depends on D151701

Comment 15

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Don't crash when buffer_unmap returns an error. r=jimb

Depends on D151702

Comment 16

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Differentiate between destroying and dropping a buffer. r=jimb

The former frees resources but keeps the handle. It can be called multiple times. The latter destroys the handle. Any subsequent reference to the same buffer is a bug and will cause the GPU process to crash.

Depends on D152080

Comment 17

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Ensure WebGPUParent outlives the map callback. r=jimb

Depends on D152081

Comment 18

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Remove the public/private/public sandwich in Buffer.h. r=jimb

Depends on D152082

Comment 19

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Rename a variable to make the destination of the memcpy more explicit. r=jimb

Depends on D152093

Comment 20

[Phabricator Automation]

Comment on attachment 9285119 [details]
Bug 1777535 - Move the buffer creation code in Buffer.cpp. r=jimb

Revision D151617 was moved to bug 1780792. Setting attachment 9285119 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9285137 [details]
Bug 1777535 - Remove Buffer::Mappable(). r=jimb

Revision D151630 was moved to bug 1780792. Setting attachment 9285137 [details] to obsolete.

Comment 22

[Phabricator Automation]

Comment on attachment 9285138 [details]
Bug 1777535 - use Buffer::GetDevice instead of mParent. r=jimb

Revision D151631 was moved to bug 1780792. Setting attachment 9285138 [details] to obsolete.

Comment 23

[Phabricator Automation]

Comment on attachment 9285139 [details]
Bug 1777535 - Move the remaining buffer logic in Device.cpp into Buffer.cpp. r=jimb

Revision D151632 was moved to bug 1780792. Setting attachment 9285139 [details] to obsolete.

Comment 24

[Phabricator Automation]

Comment on attachment 9285916 [details]
Bug 1777535 - Remove the public/private/public sandwich in Buffer.h. r=jimb

Revision D152093 was moved to bug 1780792. Setting attachment 9285916 [details] to obsolete.

Comment 25

[Phabricator Automation]

Comment on attachment 9286045 [details]
Bug 1777535 - Rename a variable to make the destination of the memcpy more explicit. r=jimb

Revision D152175 was moved to bug 1780792. Setting attachment 9286045 [details] to obsolete.

Comment 26

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Mapped at creation implies write access. r=jimb

Depends on D152175

Comment 27

[Jim Blandy :jimb]

Landing failed because we didn't request webidl review.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0da5289fe5a9
Return pointer and length in wgpu_server_buffer_get_mapped_range. r=jimb
https://hg.mozilla.org/integration/autoland/rev/6f732421a0b4
Use unsafe shmems. r=jimb
https://hg.mozilla.org/integration/autoland/rev/85ac57add037
Map only the required buffer range on the Parent side. r=jimb
https://hg.mozilla.org/integration/autoland/rev/d2f4d878d51f
Track the buffer mapAsync promise. r=jimb
https://hg.mozilla.org/integration/autoland/rev/446e62674d9d
Validate getMappedRange against the actually mapped range. r=jimb
https://hg.mozilla.org/integration/autoland/rev/e312cdad1fee
Unmap the buffer in Destroy. r=jimb,emilio
https://hg.mozilla.org/integration/autoland/rev/219760e8c20e
Simplify Buffer::Cleanup. r=jimb
https://hg.mozilla.org/integration/autoland/rev/f674057a477f
Validate mapAync mode on the parent side. r=jimb
https://hg.mozilla.org/integration/autoland/rev/aa8e1c7f727f
Don't crash when buffer_map_async returns an error. r=jimb
https://hg.mozilla.org/integration/autoland/rev/fe75bdc54a31
Don't crash when buffer_unmap returns an error. r=jimb
https://hg.mozilla.org/integration/autoland/rev/de8da0f89c50
Differentiate between destroying and dropping a buffer. r=jimb
https://hg.mozilla.org/integration/autoland/rev/89450745f60b
Ensure WebGPUParent outlives the map callback. r=jimb
https://hg.mozilla.org/integration/autoland/rev/5bef755ea09b
Mapped at creation implies write access. r=jimb

Comment 29

[Marian-Vasile Laza]

Backed out 22 changesets (Bug 1780792, Bug 1778713, Bug 1771254, Bug 1777535) for causing bustages on WebGPUParent.h.
Backout link
Push with failures <--> BP-hybrid
Failure Log

Comment 30

[Nicolas Silva [:nical]]

Attachment: Bug 1777535 - Work around build issues. r=jimb,jgilbert

I think that this issue was introduced by another series of patch (when we started passing nsACStrings as parameter in the bindings), but were hidden by unified builds.

Depends on D152521

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ad9e8142723
Return pointer and length in wgpu_server_buffer_get_mapped_range. r=jimb
https://hg.mozilla.org/integration/autoland/rev/4f2dde9ea2d7
Use unsafe shmems. r=jimb
https://hg.mozilla.org/integration/autoland/rev/fee5c1cf6b18
Map only the required buffer range on the Parent side. r=jimb
https://hg.mozilla.org/integration/autoland/rev/8d3dd35ca52f
Track the buffer mapAsync promise. r=jimb
https://hg.mozilla.org/integration/autoland/rev/23f5249c9779
Validate getMappedRange against the actually mapped range. r=jimb
https://hg.mozilla.org/integration/autoland/rev/40348d5f2170
Unmap the buffer in Destroy. r=jimb,emilio
https://hg.mozilla.org/integration/autoland/rev/86678ef96c5f
Simplify Buffer::Cleanup. r=jimb
https://hg.mozilla.org/integration/autoland/rev/1e20b97d3f9a
Validate mapAync mode on the parent side. r=jimb
https://hg.mozilla.org/integration/autoland/rev/cfc69fcae3fb
Don't crash when buffer_map_async returns an error. r=jimb
https://hg.mozilla.org/integration/autoland/rev/286c446e5c1b
Don't crash when buffer_unmap returns an error. r=jimb
https://hg.mozilla.org/integration/autoland/rev/b3a5f48283a5
Differentiate between destroying and dropping a buffer. r=jimb
https://hg.mozilla.org/integration/autoland/rev/64cbb7fbb19c
Ensure WebGPUParent outlives the map callback. r=jimb
https://hg.mozilla.org/integration/autoland/rev/c1d87be26dea
Mapped at creation implies write access. r=jimb
https://hg.mozilla.org/integration/autoland/rev/dcfdf3f68642
Work around build issues. r=gfx-reviewers,lsalzman

Comment 32

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8ad9e8142723
https://hg.mozilla.org/mozilla-central/rev/4f2dde9ea2d7
https://hg.mozilla.org/mozilla-central/rev/fee5c1cf6b18
https://hg.mozilla.org/mozilla-central/rev/8d3dd35ca52f
https://hg.mozilla.org/mozilla-central/rev/23f5249c9779
https://hg.mozilla.org/mozilla-central/rev/40348d5f2170
https://hg.mozilla.org/mozilla-central/rev/86678ef96c5f
https://hg.mozilla.org/mozilla-central/rev/1e20b97d3f9a
https://hg.mozilla.org/mozilla-central/rev/cfc69fcae3fb
https://hg.mozilla.org/mozilla-central/rev/286c446e5c1b
https://hg.mozilla.org/mozilla-central/rev/b3a5f48283a5
https://hg.mozilla.org/mozilla-central/rev/64cbb7fbb19c
https://hg.mozilla.org/mozilla-central/rev/c1d87be26dea
https://hg.mozilla.org/mozilla-central/rev/dcfdf3f68642