Closed Bug 1653659 Opened 5 years ago Closed 4 years ago

Sandbox hunspell using RLBox

Categories

(Core :: Spelling checker, enhancement)

Product:
Core
Component:
Spelling checker
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: deian, Assigned: deian)

References

(Blocks 2 open bugs)

Details

(Keywords: perf-alert)

Attachments

(5 files, 3 obsolete files)

Bug 1653659 - Part 1: Retrofit hunspell with RLBox for sandboxing
47 bytes, text/x-phabricator-request
Details | Review
Bug 1653659 - Part 2: Add support for Wasm sandboxing hunspell.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1653659 - Part 4: Turn on Wasm sandboxing for hunspell
47 bytes, text/x-phabricator-request
Details | Review
Bug 1653659 - Part 3: Only allow non-empty paths in RLBoxHunspell
48 bytes, text/x-phabricator-request
Details | Review
Bug 1653659 - Part 4: Turn on Wasm sandboxing for hunspell
48 bytes, text/x-phabricator-request
Details | Review

Use RLBox to WasmBox hunspell.

This patch only introduces the RLBox-wrapped hunspell (so uses the
NOOP-sandbox).

See Also: → 1403735
See Also: 1403735 →

Depends on D84007

Depends on D84007

Attachment #9168080 - Attachment is obsolete: true
Depends on: 1657573
Attachment #9164431 - Attachment description: Bug 1653659 - Part 1: Sandbox hunspell with RLBox → Bug 1653659 - Part 1: Retrofit hunspell with RLBox for sandboxing
Blocks: Wasm-sandboxed-libraries
Attachment #9168262 - Attachment description: Bug 1653659 - Part 2: Turn on Wasm sandboxing for hunspell → Bug 1653659 - Part 2: Add support for Wasm sandboxing hunspell

Depends on D86063

Attachment #9168262 - Attachment description: Bug 1653659 - Part 2: Add support for Wasm sandboxing hunspell → Bug 1653659 - Part 2: Add support for Wasm sandboxing hunspell.

Sorry for the delay with wrapping up the patches. We needed one more patch to pull in a hxx file from hunspell.

Everything should be set. And I ran @jkew's experiment:

  1. Load https://en.wikipedia.org/wiki/History_of_Western_civilization
  2. Set document.body.contentEditable = true
  3. Turn on profiler
  4. Scroll to bottom of page and click

On my machine, I can't perceive the difference in the delay.

Vanilla: https://share.firefox.dev/3lctVn6
Sandboxed: https://share.firefox.dev/2GKJvaC

When we WasmBox hunspell, MOZILLA_CLIENT is undefined and, so, hunspell
needs utf_info.hxx. This patch pulls in this header file.

Depends on D86063

Attachment #9184160 - Attachment description: Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell → Bug 1653659 - Part 4: Turn on Wasm sandboxing for hunspell.

There are definitely some issues with the patchset unfortunately.

This try run does not enable wasmboxing, just adds the first 3 patches:
https://treeherder.mozilla.org/jobs?repo=try&revision=b98af45327616998a6b6f75d529a17445db2841b&selectedTaskRun=C-LgYRZHQsug7er7TmKkGQ.0
https://treeherder.mozilla.org/push-health?repo=try&revision=b98af45327616998a6b6f75d529a17445db2841b&testGroup=pr&selectedTest=arenatDallocSmallarenachunktvoidarenachunkmapt&selectedTaskId=

  • A lot of jemalloc failures; two different signatures
  • 3 Android build failures (all looking like the same issue)
Flags: needinfo?(deian)

Fixed the double free. Sorry about that. hunspell_destroy freed the pointer so we didn't need to free_in_sandbox.

The build failure is a template specialization issue in RLBox. Creating separate bug for that.

Flags: needinfo?(deian)
Depends on: 1683054

Henry: Any chance I can get your take on us using hunspell with MOZ_CLIENT off? This would be using hunspell's utf table instead of the existing one in Firefox.

Flags: needinfo?(hsivonen)

What impact would that have on the overall size?

Looking at the profiler data from https://bugzilla.mozilla.org/show_bug.cgi?id=1653659#c5 it seems like the memory usage is comparable.

(In reply to Deian Stefan from comment #11)

Looking at the profiler data from https://bugzilla.mozilla.org/show_bug.cgi?id=1653659#c5 it seems like the memory usage is comparable.

I think Jonathan was talking about code size rather than memory usage.

Roughly a 1M increase in the bunzip:

$ du  ./(wasm|vanilla)-build/dist/firefox-86.0a1.en-US.linux-x86_64.tar.bz2
102168  ./vanilla-build/dist/firefox-86.0a1.en-US.linux-x86_64.tar.bz2
102816  ./wasm-build/dist/firefox-86.0a1.en-US.linux-x86_64.tar.bz2

It seems unfortunate to increase the size so much (I think that'll equate to about a 2% increase in the download package size on Android, for example), if it's largely coming from duplication of character data/encoding tables that we already have in gecko. Is there some way we can avoid that?

(In reply to Jonathan Kew (:jfkthame) from comment #15)

It seems unfortunate to increase the size so much (I think that'll equate to about a 2% increase in the download package size on Android, for example),

Particularly because I don't think we use hunspell on Android at all. We could special-case the platform, but I think we'd rather avoid a 1M increase of the Desktop installer too.

if it's largely coming from duplication of character data/encoding tables that we already have in gecko. Is there some way we can avoid that?

Yeah seems like if these are just read-only data tables, should be straightforward to expose them to the sandbox somehow?

(In reply to Bobby Holley (:bholley) from comment #16)

(In reply to Jonathan Kew (:jfkthame) from comment #15)

It seems unfortunate to increase the size so much (I think that'll equate to about a 2% increase in the download package size on Android, for example),

Particularly because I don't think we use hunspell on Android at all. We could special-case the platform, but I think we'd rather avoid a 1M increase of the Desktop installer too.

I was under the impression that we may already be doing this?

if it's largely coming from duplication of character data/encoding tables that we already have in gecko. Is there some way we can avoid that?

Yeah seems like if these are just read-only data tables, should be straightforward to expose them to the sandbox somehow?

We can do it, just have to modify hunspell. We need to expose functions like ToUpperCase. Most seem reasonable enough. The gnarly bits are in get_current_cs, but we can also just pull whole function this into a trusted callback. I can take a crack at this next week. My sense is that it might be easier to try to merge some of this back into hunspell? Our changes shouldn't really extend beyond the MOZILLA_CLIENT guards anyway.

Flags: needinfo?(hsivonen)

(In reply to Deian Stefan from comment #17)

(In reply to Bobby Holley (:bholley) from comment #16)

(In reply to Jonathan Kew (:jfkthame) from comment #15)

It seems unfortunate to increase the size so much (I think that'll equate to about a 2% increase in the download package size on Android, for example),

Particularly because I don't think we use hunspell on Android at all. We could special-case the platform, but I think we'd rather avoid a 1M increase of the Desktop installer too.

I was under the impression that we may already be doing this?

if it's largely coming from duplication of character data/encoding tables that we already have in gecko. Is there some way we can avoid that?

Yeah seems like if these are just read-only data tables, should be straightforward to expose them to the sandbox somehow?

We can do it, just have to modify hunspell. We need to expose functions like ToUpperCase. Most seem reasonable enough. The gnarly bits are in get_current_cs, but we can also just pull whole function this into a trusted callback. I can take a crack at this next week. My sense is that it might be easier to try to merge some of this back into hunspell? Our changes shouldn't really extend beyond the MOZILLA_CLIENT guards anyway.

I think we should make whatever changes we need to hunspell (while obviously trying to err on the side of changing less). We can then figure out whether it's less work to either (1) upstream those changes, or (2) re-apply the changes if/when we ever pull in a hunspell update.

(In reply to Deian Stefan from comment #9)

Henry: Any chance I can get your take on us using hunspell with MOZ_CLIENT off? This would be using hunspell's utf table instead of the existing one in Firefox.

Primarily for code side reasons, my preference would be to make Gecko expose the relevant services to hunspell instead of having hunspell provide its own duplicate services.

Attachment #9184160 - Attachment description: Bug 1653659 - Part 4: Turn on Wasm sandboxing for hunspell. → Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell.
Attachment #9186534 - Attachment is obsolete: true

Got a clean try run on the first two patches and with the third.

Deian, just to confirm here, the patches do as you describe in #17, right? That is, they modify hunspell as needed so we don't duplicate the data tables and avoid increasing the binary size? If so i'll try to land patches 1 and 2.

Flags: needinfo?(deian)

Yep!

Flags: needinfo?(deian)
Keywords: leave-open
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/876dfd999d65 Part 1: Retrofit hunspell with RLBox for sandboxing r=tjr https://hg.mozilla.org/integration/autoland/rev/783310e1f5b8 Part 2: Add support for Wasm sandboxing hunspell. r=firefox-build-system-reviewers,dmajor

Shravan: Can you look at this? I thought you addressed some of these

Flags: needinfo?(deian) → needinfo?(shravanrn)

Hmm... this was fixed upstream rlbox, but the in-tree rlbox doesn't seem to have been updated. I need to follow up as to why.
But for now you can update your in-tree rlbox by running

cd mozilla-central/third_party/rlbox/
./update.sh

If you submit a build after this, the build should succeed

Flags: needinfo?(shravanrn)

Oh, looks like the rlbox update hasn't been landed yet
https://phabricator.services.mozilla.com/D99989

@tom; Could you help with this?

Flags: needinfo?(tom)

landed, will land this after the soft freeze

Flags: needinfo?(tom)
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3f9c23c365cb Part 1: Retrofit hunspell with RLBox for sandboxing r=tjr https://hg.mozilla.org/integration/autoland/rev/9f74560eb1c0 Part 2: Add support for Wasm sandboxing hunspell. r=firefox-build-system-reviewers,dmajor

I did a perf measurement

wasmbox: https://treeherder.mozilla.org/jobs?repo=try&revision=af138a2d8f5efe2aa8f15413191edee09b5eaaf8&selectedTaskRun=HQtufjEoQbyURu8Q5WlKHA.0
control: https://treeherder.mozilla.org/jobs?repo=try&revision=c7ff702e57508ec6e9f7b53364c62f49f830c6df&selectedTaskRun=Kq1m0dd9TdO0rR1eWlBZ2A.0

wasmbox control
1673 2440
1851 1944
1405 852
1460 1832
1309 1289

1539.6 1671.4

I pasted the contents of document.cpp (from searchfox) into a textarea with extensions.spellcheck.inline.max-misspellings set to 500000. I recorded the time for DoSpellCheck in the profiler on my Macbook.

I can't entirely explain the differences here, maybe the perf differences would be exposed after a lot more trials, but it doesn't seem like wasmboxing hunspell produces a crazy difference.

I recorded the time for DoSpellCheck in the profiler on my Macbook.

Isn't DoSpellCheck on the content-process side of things, which would be essentially unaffected here? AIUI, the actual use of hunspell happens in the parent via IPC, so the question would be what difference (if any) is there in parent-process time. Whatever extra time/work the sandboxing is costing would show up there, not in the content process. Maybe try looking at something like mozSpellChecker::CheckWord in the parent?

(In reply to Jonathan Kew (:jfkthame) from comment #32)

Whatever extra time/work the sandboxing is costing would show up there, not in the content process. Maybe try looking at something like mozSpellChecker::CheckWord in the parent?

Thanks! I checked CheckWord in the parent process and.... wasmbox is twice as fast. ¯_(ツ)_/¯

wasmbox	control
48	111
55.7	98.7
39.7	101
43.7	105
33.5	118
	
44.12	106.74

That's curious.... I mean, that's great -- but it would be nice to understand why! (Do we end up using an entirely different allocator or something like that? I think hunspell does quite a bit of malloc/free-ing, so if the wasm'd version gets a better-performing allocator that could make a difference.)

Yep, that's probably it. In the sandbox we can't use the moz_alloc and free.

Now that we're in the new cycle, do you have any concerns about enabling this and letting it ride the trains?

Flags: needinfo?(jfkthame)

Seems like it should be fine -- let's go for it.

Flags: needinfo?(jfkthame)

@tjr rebased, good to go!

Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a137de215994 Part 3: Turn on Wasm sandboxing for hunspell. r=dmajor

(In reply to Pulsebot from comment #39)

Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a137de215994
Part 3: Turn on Wasm sandboxing for hunspell. r=dmajor

== Change summary for alert #29385 (as of Mon, 22 Mar 2021 18:27:57 GMT) ==

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
1% installer size osx-cross 79,991,997.25 -> 80,452,306.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=29385

I don't know why; but sure enough on OSX there's a consistent fail Expect word foobar is spelled correctly with the dictionary nosuggest on OSX.

The OSX Bpgo failure seems to be a packaging issue, I can dig into that.

Attachment #9184160 - Attachment description: Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell. → WIP: Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell.
Attachment #9184160 - Attachment description: WIP: Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell. → Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell.
Flags: needinfo?(deian)
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1bbf93c8f424 Part 3: Turn on Wasm sandboxing for hunspell. r=dmajor

Backed out changeset 1bbf93c8f424 (Bug 1653659) for causing xpcshell failures in test_hunspell.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/cbae78ec871e1b698f2051dd7cf0aeba0b5a9475
Push with failures, failure log.

Flags: needinfo?(deian)

(In reply to Pulsebot from comment #44)

Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bbf93c8f424
Part 3: Turn on Wasm sandboxing for hunspell. r=dmajor

== Change summary for alert #29494 (as of Mon, 29 Mar 2021 14:44:23 GMT) ==

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
1% installer size osx-cross 80,091,867.58 -> 80,640,593.08

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=29494

Depends on: 1710685

Depends on D114924

Attachment #9184160 - Attachment description: Bug 1653659 - Part 3: Turn on Wasm sandboxing for hunspell. → Bug 1653659 - Part 4: Turn on Wasm sandboxing for hunspell

New patches should address the bug. Kicked off try: https://treeherder.mozilla.org/jobs?repo=try&revision=d5df9facfb9c4bcf711f5d5206a10d81fc7a95c8

Flags: needinfo?(deian)
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fac526423ccb Part 3: Safely handle empty strings in RLBoxHunspell r=tjr https://hg.mozilla.org/integration/autoland/rev/523e4bc2aa47 Part 4: Turn on Wasm sandboxing for hunspell r=dmajor
Attachment #9221495 - Attachment description: Bug 1653659 - Part 3: Safely handle empty strings in RLBoxHunspell → Bug 1653659 - Part 3: Only allow non-empty paths in RLBoxHunspell
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b187a1b158df Part 3: Only allow non-empty paths in RLBoxHunspell r=tjr https://hg.mozilla.org/integration/autoland/rev/018859776d85 Part 4: Turn on Wasm sandboxing for hunspell r=dmajor

Sorry for all the backouts. We need to land https://bugzilla.mozilla.org/show_bug.cgi?id=1710685 before we can land this.

Flags: needinfo?(deian)

(In reply to Pulsebot from comment #49)

Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fac526423ccb
Part 3: Safely handle empty strings in RLBoxHunspell r=tjr
https://hg.mozilla.org/integration/autoland/rev/523e4bc2aa47
Part 4: Turn on Wasm sandboxing for hunspell r=dmajor

== Change summary for alert #30094 (as of Fri, 14 May 2021 16:50:48 GMT) ==

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
1% installer size osx-cross 79,883,220.08 -> 80,415,397.50

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30094

(In reply to Pulsebot from comment #51)

Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b187a1b158df
Part 3: Only allow non-empty paths in RLBoxHunspell r=tjr
https://hg.mozilla.org/integration/autoland/rev/018859776d85
Part 4: Turn on Wasm sandboxing for hunspell r=dmajor

== Change summary for alert #30108 (as of Mon, 17 May 2021 18:37:39 GMT) ==

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
1% installer size osx-cross 79,880,551.85 -> 80,485,449.42

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30108

Depends on D116545

Depends on D116547

https://treeherder.mozilla.org/jobs?repo=try&revision=db6cff02184d0cc1dff5285acf5eff71d074b213

Can we abandon part 3 and 4 phabs (I can't access them anymore).

Attachment #9221495 - Attachment is obsolete: true
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78780f84b13a Part 3: Only allow non-empty paths in RLBoxHunspell r=tjr
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9edd295ddb08 Part 4: Turn on Wasm sandboxing for hunspell r=tjr

I think the leave-open here was just an oversight and should have been cleared. Resolving as fixed.

Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: