1693271 - Switch MatchPattern and MatchGlob to use the Rust regex engine

Status: RESOLVED FIXED

(WebExtensions :: General, task, P3)

Description

[:Gijs (back August 25; he/him)]

In bug 1691079 users reported a hang with a specific URL containing a lot of consecutive * characters that turned out to be related to an add-on (wappalyzer).

As Rob noted there:

It's not uncommon to blindly pass the URL to chrome.tabs.query without being aware of the impact of *, so we probably need to find a solution for this.

We have a TODO to consider a different regexp engine at https://searchfox.org/mozilla-central/rev/5120ec68572d946bd15101cf2ee2eaf4a210724f/toolkit/components/extensions/MatchPattern.cpp#661-663

We could probably change the replacing patterns to replace /\*+/ with .* instead of replacing each individual * to fix the impact in bug 1691079, but the example in bug 1690758 (7*7*7*...) would probably still explode the regex engine in unhelpful ways.

We could also introduce some kind of limit on the number of wildcards, or provide a separate API / opt-in to having the wildcard behaviour (and doing literal matching otherwise).

None of these ideas seem particularly appealing, so I hope that there's a better one that I'm missing...

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

While reading comment 0 I was 100% sure that I did already looked into this exact issue in the past and so I looked for the existing bugzilla issue:

this seems to be an exact duplicate of Bug 1570868, which is also related to a browser hang that was happening with the wapplyzer extension installed because of url that contains a lot of *.

• Bug 1570868 comment 16 contains some more details from when I did investigate the issue (and in Bug 1570868 comment 17 and Bug 1570868 comment 18 a small code snippet and a small test extension to reproduce the issue consistently without the entire wapplyzer

• In Bug 1570868 comment 24 and Bug 1570868 comment 25 Jason Orendorff did also reply to our needinfo with his perspective on the issue

Given that Bug 1570868 does already contain more details about the issue, it may be reasonable to keep Bug 1570868 and mark this one as a duplicate of that one.

Comment 3

[Kris Maglione [:kmag]]

At this point, it would probably be easy enough to switch to the Rust engine.

Comment 4

[Tomislav Jovanovic :zombie]

I fixed the simple "common" case of redundant wildcards in bug 1570868, so reopening this bug as a "use rust regex engine instead".

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Taking this as part of bug 1793995

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1693271 - Part 1: Use RustRegex for MatchGlob, r=kmag!

This also involves making MatchGlob operate on UTF8String instead of DOMString,
as the rust regex crate operates on utf-8 strings. This should have no
functional impact on callers.

Depends on D158876

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1693271 - Part 2: Fix test for slower first-run debug execution of RustRegex, r=kmag!

In debug mode, the compile for some regular expressions is much slower with the
regex crate than it was with the JS engine, likely due to the regex's crate
heavy reliance on inlining and other llvm optimizations. Subsequent runs are
much faster, as they do not need to optimize and build the DFA.

Depends on D158877

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77c06db920fe
Part 1: Use RustRegex for MatchGlob, r=kmag
https://hg.mozilla.org/integration/autoland/rev/8b71c15f24c7
Part 2: Fix test for slower first-run debug execution of RustRegex, r=kmag

Comment 9

[Cristian Tuns]

Backed out for causing build bustages on RustRegex.h

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/workspace/obj-build/dist/include/mozilla/RustRegex.h:630:24: error: unused variable 'view' [-Werror,-Wunused-variable]

Log tier 2: https://treeherder.mozilla.org/logviewer?job_id=392996025&repo=autoland

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84e7bf515c94
Part 1: Use RustRegex for MatchGlob, r=kmag
https://hg.mozilla.org/integration/autoland/rev/10c5cf69b3f0
Part 2: Fix test for slower first-run debug execution of RustRegex, r=kmag

Comment 11

[Cristian Tuns]

Backed out for causing build bustages

• Backout link
• Push with failures
• Failure Log
• Failure line: clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
  gmake[4]: *** [/builds/worker/checkouts/gecko/config/makefiles/rust.mk:433: force-cargo-library-build] Error 101

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a62d761ebd6
Part 1: Use RustRegex for MatchGlob, r=kmag
https://hg.mozilla.org/integration/autoland/rev/1f61b8f989f9
Part 2: Fix test for slower first-run debug execution of RustRegex, r=kmag

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e41b744df6b
Fix for test_MatchPattern.js xpcshell failures. CLOSED TREE

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7a62d761ebd6
https://hg.mozilla.org/mozilla-central/rev/1f61b8f989f9
https://hg.mozilla.org/mozilla-central/rev/1e41b744df6b