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

LibWebView: Crashes viewing the source of the acid1 test #3169

Open
trflynn89 opened this issue Jan 7, 2025 · 3 comments · May be fixed by #3181
Open

LibWebView: Crashes viewing the source of the acid1 test #3169

trflynn89 opened this issue Jan 7, 2025 · 3 comments · May be fixed by #3181
Labels
bug Something isn't working has repro We have a way to reproduce this bug.

Comments

@trflynn89
Copy link
Contributor

https://www.w3.org/Style/CSS/Test/CSS1/current/test5526c.htm

"View Source" on this page and see the following stack:

VERIFICATION FAILED: i < m_size at /Users/flynn/workspace/ladybird/AK/Vector.h:148

0   liblagom-ak.0.0.0.dylib       	       0x102e672ec ak_trap + 628 (Assertions.cpp:100)
1   liblagom-ak.0.0.0.dylib       	       0x102e673c4 ak_verification_failed + 176 (Assertions.cpp:110)
2   liblagom-webview.0.0.0.dylib  	       0x1029f3d30 at + 12 (Vector.h:148) [inlined]
3   liblagom-webview.0.0.0.dylib  	       0x1029f3d30 operator[] + 12 (Vector.h:156) [inlined]
4   liblagom-webview.0.0.0.dylib  	       0x1029f3d30 WebView::SourceDocument::line(unsigned long) + 52 (SourceHighlighter.cpp:30)
5   liblagom-syntax.0.0.0.dylib   	       0x1028da758 Syntax::Document::set_folding_regions(AK::Vector<Syntax::TextDocumentFoldingRegion, 0ul>) + 280 (Document.cpp:323)
6   liblagom-webview.0.0.0.dylib  	       0x1029f4510 WebView::SourceHighlighterClient::highlighter_did_set_folding_regions(AK::Vector<Syntax::TextDocumentFoldingRegion, 0ul>) + 68 (SourceHighlighter.cpp:114)
7   liblagom-web.0.0.0.dylib      	       0x104aa6f64 do_set_folding_regions + 28 (HighlighterClient.h:36) [inlined]
8   liblagom-web.0.0.0.dylib      	       0x104aa6f64 Web::HTML::SyntaxHighlighter::rehighlight(Gfx::Palette const&) + 3220 (SyntaxHighlighter.cpp:184)
9   liblagom-webview.0.0.0.dylib  	       0x1029f405c WebView::SourceHighlighterClient::SourceHighlighterClient(AK::StringView, Syntax::Language) + 760 (SourceHighlighter.cpp:63)
10  liblagom-webview.0.0.0.dylib  	       0x1029f459c SourceHighlighterClient + 20 (SourceHighlighter.cpp:40) [inlined]
11  liblagom-webview.0.0.0.dylib  	       0x1029f459c WebView::highlight_source(URL::URL const&, URL::URL const&, AK::StringView, Syntax::Language, WebView::HighlightOutputMode) + 72 (SourceHighlighter.cpp:119)

CC @AtkinsSJ

@trflynn89 trflynn89 added bug Something isn't working has repro We have a way to reproduce this bug. labels Jan 7, 2025
@tcl3
Copy link
Member

tcl3 commented Jan 7, 2025

The file in question has \r line endings. The crash didn't happen after I replaced the line endings with \n using sed.

@AtkinsSJ
Copy link
Member

AtkinsSJ commented Jan 7, 2025

Interesting... I swapped them with a hex editor and that didn't solve the crash. Nor could I reproduce the crash with a small test file using \r.

@AtkinsSJ
Copy link
Member

AtkinsSJ commented Jan 7, 2025

Smallest repro:

<style>
.foo {
hello
}
</style>

(with \r newlines)

Not sure what happened earlier, maybe I messed something else up.

AtkinsSJ added a commit to AtkinsSJ/ladybird that referenced this issue Jan 8, 2025
The previous code to determine the SourceDocument's lines was too naive:
the source text can contain other newline characters and sequences, and
the HTML/CSS/JS syntax highlighters would take those into account when
determining what line a token is on. This disagreement would cause
incorrect highlighting, or even crashes, if the source doesn't solely
use `\n` for its newlines.

In order to have everyone agree on what a line is, this patch first
processes the source to replace all newlines with `\n`. The need to
copy the source like this is unfortunate, but viewing the source is a
rare enough action that this should not cause any noticeable
performance problems.

Fixes LadybirdBrowser#3169
AtkinsSJ added a commit to AtkinsSJ/ladybird that referenced this issue Jan 13, 2025
The previous code to determine the SourceDocument's lines was too naive:
the source text can contain other newline characters and sequences, and
the HTML/CSS/JS syntax highlighters would take those into account when
determining what line a token is on. This disagreement would cause
incorrect highlighting, or even crashes, if the source didn't solely use
`\n` for its newlines.

In order to have everyone agree on what a line is, this patch first
processes the source to replace all newlines with `\n`. The need to
copy the source like this is unfortunate, but viewing the source is a
rare enough action that this should not cause any noticeable
performance problems.

As the callers have a String, and we want a String, this also changes
the function parameters to keep the source as a String instead of
converting it to StringView and back.

Fixes LadybirdBrowser#3169
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has repro We have a way to reproduce this bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants