1536466 - Multiple "ASSERTION: nsTDependentString must wrap only null-terminated strings. ... " while profiling with WebRender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Beth Rennie [:beth] (she/her)]

While profiling with WebRender enabled on a debug build, multiple of the following assertions occur:

[Parent 5439, Renderer] ###!!! ASSERTION: nsTDependentString must wrap only null-terminated strings. You are probably looking for nsTDependentSubstring.: 'this->mData[substring_type::mLength] == 0', file /home/barret/Workspace/src/hg.mozilla.org/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTString.h, line 456

STR:

1. Build a debug build of Firefox.
2. Enable Web Render.
3. Start profiling with the Gecko Profiler.

Expected results:
We should not be hitting this assertion.

After some investigation, this occurs because <GeckoProfilerHooks as ProfilerHooks>::add_text_marker in the WebRender bindings accepts a &str and passes its pointer and length to gecko_profiler_add_text_marker, which constructs a nsTDependentString. However, Rust strings are not NUL-terminated.

Comment 1

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1536466 - Fix nsTDependentString with non-null-terminated buffer assertion r?bholley

gecko_profiler_add_text_marker was being passed a character pointer and a
length to construct a nsDependentCString. However, these values were coming
from a Rust &str, which is not null-terminated, causing an debug assertion to
be hit (and possible memory safety issues if mishandle the string). We now
construct an nsDependentCSubstring instead.

Comment 2

[Bobby Holley (:bholley)]

[Tracking Requested - why for this release]:

We should uplift this to 67, otherwise we have a potential out-of-bounds read with the profiler enabled.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd2b7f779973
Fix nsTDependentString with non-null-terminated buffer assertion r=bholley

Comment 4

[Oana Pop-Rus]

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

Comment 5

[Pascal Chevrel:pascalc]

Tracking for 67, please request an uplift when you get a chance, thanks.

Comment 6

[Beth Rennie [:beth] (she/her)]

Comment on attachment 9052007 [details]
Bug 1536466 - Fix nsTDependentString with non-null-terminated buffer assertion r?bholley

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1532810
• User impact if declined: Users with WebRender enabled that also are using the Gecko Profiler might experience crashes while profiling
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is not risky because it touches a codepath that is only hit while users are running with Web Render enabled and collecting a profile with the Gecko Profiler. Even then, the change is very small in scope and well understood.
• String changes made/needed:

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9052007 [details]
Bug 1536466 - Fix nsTDependentString with non-null-terminated buffer assertion r?bholley

Minimal patch to prevent crashes in webrender / gecko profiler interactions, approved for the next beta, thanks.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2211f0befe54