Munter / subfont

Command line tool to optimize your webfont loading. Aggressive subsetting based on your font use, self-hosting of Google fonts and preloading
MIT License
1.56k stars 29 forks source link

memory access out of bounds #145

Closed Lionad-Morotar closed 3 years ago

Lionad-Morotar commented 3 years ago

报错信息:

╰❯❯❯ npm run subfont

> fontmin-tool@1.0.0 subfont D:\@Github\blog-font-cdn
> subfont ./lab/7000.html -o ./tmp

Guessing --root from input files: file:///D:/@Github/blog-font-cdn/lab/
 ✔ 0.005 secs: logEvents
 ✔ 0.080 secs: loadAssets
 ✔ 0.019 secs: populate
 ✔ 0.001 secs: checkIncompatibleTypes
 ✔ 0.001 secs: applySourceMaps
 ✔ 0.002 secs: populate
 ✔ 0.001 secs: populate
 ⚠ WARN: lab\source\SourceHanSerifCN-SemiBold.otf - memory access out of bounds
         Including assets:
             lab\index.css

 ⚠ WARN: lab\source\SourceHanSerifCN-SemiBold.otf - memory access out of bounds
         Including assets:
             lab\index.css

 ✔ 0.019 secs: compressJavaScript
 ✔ 0.000 secs: writeAssetsToDisc
HTML/JS/CSS size increase: 168 B
Total savings: -168 B
Output written to file:///D:/@Github/blog-font-cdn/tmp/

错误来源: image

重现方式: https://github.com/Lionad-Morotar/font-cdn

papandreou commented 3 years ago

@Lionad-Morotar, thanks for reporting this. I think it's a problem with one of the wasm-based tools we use for converting and subsetting fonts. I'll try to take a look soon.

papandreou commented 3 years ago

If you'd like to expedite the process, could you try to see if you can reproduce the problem with the involved font(s) with just https://github.com/papandreou/fontverter (https://github.com/fontello/wawoff2 / https://github.com/odemiral/woff2sfnt-sfnt2woff) or https://github.com/papandreou/subset-font in the picture?

Lionad-Morotar commented 3 years ago

看起来像是 WebAssembly 的错误: image

papandreou commented 3 years ago

Great, thanks! Then it seems like the problem is in https://github.com/harfbuzz/harfbuzzjs or our use of it.

@ebraminio, does this ring a bell? If not I can do some triaging tonight (CEST)

ebraminio commented 3 years ago

does this ring a bell? If not I can do some triaging tonight (CEST)

We have in exports.memory.grow(400); // each page is 64kb in size in https://github.com/papandreou/subset-font/blob/fce181b/index.js#L10 and CJK fonts like that need much larger space, implementing dynamic memory growing in harfbuzzjs was possible but I tbh failed to figure it out yet having static defined / max memory size by user is a feature by its own (well, falls into "it's a feature not a bug…"), one solution also can be implementing the whole thing in emscripten which provides this by default which I preferred to not go with to control more the resulting .wasm file and skip the emscripten wrapper which could have done by some other no so clean ways now I see, guess for now increasing the memory (the referred line) probably would be best, the font is 10mb and maybe some 100mb would be enough for now (but there are also much bigger CJK fonts also, maybe you can extend dynamically given the font size also)

papandreou commented 3 years ago

@Lionad-Morotar, can you try to play with other values by locally changing that exports.memory.grow(400) line to see if that makes it work? Eg. to 2000?

papandreou commented 3 years ago

I can reproduce the error using https://github.com/Lionad-Morotar/font-cdn, except that my node crashes with Bus error: 10 instead of reporting the error.

Changing 400 to 2000 fixes it for me, so I released subset-font 1.2.3 / subfont 6.0.2 with that change.

I played around with growing the WebAssembly heap dynamically based on the size of the original font, but then I ran into Cannot perform %TypedArray%.prototype.set on a detached ArrayBuffer. Seems like it'd require a new instantiation of hb-subset.wasm for each subsetting operation, which seems excessive.

Btw. @Lionad-Morotar, subfont no longer requires fonttools etc., so you can remove https://github.com/Lionad-Morotar/font-cdn/blob/0b0461f96fbf79389772a3bc359856cdd6d10b14/package.json#L7 🤗

Thanks for reporting the issue and @ebraminio for helping out!

Lionad-Morotar commented 3 years ago

It works ! Thanks !

yisibl commented 3 years ago

@ebraminio I submitted an issue https://github.com/harfbuzz/harfbuzzjs/issues/39