1479196 - Browser crashes with a html file full of emojis, if you drag-and-drop the file from desktop to browser

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180727231224

Steps to reproduce:

create new nightly profile
enable WR
open the attached html file


Actual results:

instant crash
https://crash-stats.mozilla.com/report/index/e5f1483f-6d16-410c-933d-61d8e0180728


Expected results:

no crash

Without WR, the page uses massive amount of memory, OOM, or just hangs

Comment 1

[Mayank Bansal]

Attachment: em.html

Comment 2

[Mayank Bansal]

Forgot to add that you need to enable WR

Comment 3

[Mayank Bansal]

So, this may not be a WR bug per se

If you open the attachement directly as the attachment, it opens fine
BUT, if you download the attachment, and then drag-and-drop it into the browser, you get a crash.

Comment 4

[Mayank Bansal]

New STR:

1. create new profile. Enable WR
2. Download the attachment to your desktop
3. Drag-and-drop the file into the browser
OR
menu bar->file->open file and then browse to the downloaded attachment

Comment 6

[YF (Yang)]

Unfortunately, I still can't reproduce it with WebRender enabled and the file downloaded.

Comment 7

[Mayank Bansal]

(In reply to YF (Yang) from comment #6)
> Unfortunately, I still can't reproduce it with WebRender enabled and the
> file downloaded.

not sure why. Im on Winx64, with Intel gfx
If you do the STR without WR, then the page hangs/rapidly increases memory for me

Comment 8

[Mayank Bansal]

Attachment: memory-report.json.gz

Comment 9

[Mayank Bansal]

Attachment: aboutsupport.txt

Comment 10

[Alice0775 White]

Attachment: em.html specified wrong charset(other than UTF-8)

I think this seems to be html parser problem.

If wrong charset is specified,
With WebRender:  tab crashes
Without WebReder: browser become unresponsive for long period. and finally render text(Of course it is mojibake)

Comment 11

[Alice0775 White]

In nightly63.0a1, 
If wrong charset is specified (attachment 8995725 [details]),
With WebRender:  tab crashes
Without WebReder: browser become unresponsive for long period. and finally render text(Of course it is mojibake)


Un affected on Firefox62.0b12 and release61 and 60ESR.

Comment 12

[Alice0775 White]

(In reply to YF (Yang) from comment #6)
> Unfortunately, I still can't reproduce it with WebRender enabled and the
> file downloaded.


In your environment, because browser detected charset successfully.
Please try attachment 8995725 [details].

Comment 13

[Alice0775 White]

Regression window(crash with WebRender):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=379a933da614e9176070e28522f486ea18ded5e1&tochange=36bc055f7ad075c17fd03781f1f1a074b00ccb42

regressed by: 36bc055f7ad0	Lee Salzman — Bug 1400917 - render missing glyphs with WebRender. r=gankro

:lsalzman,
Your patch seems to cause the crash, Could you look into this?


(about performance problem for non-WebRender, I will file an another new bug later)

Comment 14

[Mayank Bansal]

any idea why there is a behavioural difference between opening the file directly from the bugzilla link, and downloading it and then opening it in browser?

Comment 15

[Alice0775 White]

(In reply to Mayank Bansal from comment #14)
> any idea why there is a behavioural difference between opening the file
> directly from the bugzilla link, and downloading it and then opening it in
> browser?

Probably, the difference is auto-charset detection.


I can reproduce when open link in the bug and bookmark as well as D&D with attachment 8995725 [details].

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Alice0775 White from comment #13)
> Regression window(crash with WebRender):
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=379a933da614e9176070e28522f486ea18ded5e1&tochange=36bc
> 055f7ad075c17fd03781f1f1a074b00ccb42
> 
> regressed by: 36bc055f7ad0	Lee Salzman — Bug 1400917 - render missing glyphs
> with WebRender. r=gankro
> 
> :lsalzman,
> Your patch seems to cause the crash, Could you look into this?

Specifically, it looks like when there are lots of missing-glyph hexboxes to draw, we crash with "IPC message size is too large":

https://crash-stats.mozilla.com/report/index/27c7e9a5-0d01-47c9-902f-082370180730

Comment 17

[Jim Mathies [:jimm]]

Hey David, this looks like a great stress test related to web renderer and page rendering - an IPC message from WR is hitting a max size constraint. Seems like something your team might want to investigate for usefulness.

Comment 18

[Lee Salzman [:lsalzman]]

I will look into optimizing the tofu rendering so that we're not bumping into the IPC message limit so easily.

Comment 19

[Lee Salzman [:lsalzman]]

Attachment: render missing glyphs with rectangle runs

This should drop the number of rectangles we're sending to WR down to half of what we were sending before, which should substantially reduce the memory/serialization pressure we're experiencing now. While eventually we probably want to move past the rectangle approach, this will at least keep us going for a while, I think, until then.

Comment 20

[Jonathan Kew [:jfkthame]]

While this should reduce the pressure, it doesn't seem to go far enough to really resolve the issue here; I tried applying the patch locally, and still crash if I try to load the example page with incorrect encoding (resulting in lots of hexboxes) and webrender enabled.

Could we arrange to flush the WR pipeline periodically if a bunch of missing glyphs are being drawn, so that we don't accumulate many thousands of them into a single IPC message?

Comment 21

[Jonathan Kew [:jfkthame]]

ISTM that drawing missing-glyph boxes with WR is currently not at all performant, and unless we can substantially improve it, our best option for the moment might be to fall back to non-WR rendering (revert bug 1400917) for now.

As an experiment, I tried simplifying further commenting-out the drawing of the hex digits altogether, so that we just draw an empty box for each missing glyph. This was enough to avoid the crash with the mis-encoded example here, but the rendering feels distinctly laggy.

Here's a profile of reloading the page (from a local file) with WR enabled:

  https://perfht.ml/2nklaej

For comparison, with WR disabled, the page renders much more snappily; here's the corresponding profile:

  https://perfht.ml/2nkJhcQ

It seems clear from the WR-enabled profile that painting lots of missing-glyphs, even when they're simplified to the point of just being empty rectangles, is really hurting us. :(

Comment 22

[Lee Salzman [:lsalzman]]

Attachment: add support to TextDrawTarget for defining and pushing WR images

This builds off the support for WR color-masked images in https://github.com/servo/webrender/pull/2969 and allows the defining and use of WR images necessary for implementing the missing glyph atlas in a subsequent patch.

The parameters necessary to plumb that don't exactly fit into the normal DrawTarget interface, so in the spirit of avoiding square-peg/round-hole hacks, this just adds straight-forward DefineImage and PushImage calls, as there wasn't a sane way to map DrawSurface to them unfortunately.

While I was at it, I updated StrokeRect to map to a PushBorder command instead of multiple PushLine commands, as we need all the help we can get to reduce IPC traffic here.

Comment 23

[Lee Salzman [:lsalzman]]

Attachment: draw missing glyphs from an atlas instead of rectangles

As noted, this gets rid of the rectangle approach entirely and instead tries to draw each glyph with a single DrawSurface call. This is as opposed to using MaskSurface, which isn't necessarily well-supported by all DrawTargets, or as performant, and doesn't have all the necessary scaling and filtering options required implement.

For WR, there is a bit of complexity since we have no easy way to support transforms, but on the other hand we do easily support color-masking. So for a given writing mode transform, of which there are only a few, we need to generate a new atlas on demand to source from. Otherwise, there is just basic plumbing to make sure that when we purge the atlas, we clear the associated WR image resource, or that when the WR layer manager goes away, we clean up any stale WR image keys that were getting used with WR.

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 8999725 [details] [diff] [review]
draw missing glyphs from an atlas instead of rectangles

Review of attachment 8999725 [details] [diff] [review]:
-----------------------------------------------------------------

In general this looks like a good step forward, but I'm concerned about the use of a number of global variables to maintain state. Can you rework it to avoid that issue (or explain why it isn't actually a problem)?

::: gfx/thebes/gfxFontMissingGlyphs.cpp
@@ +76,5 @@
> +
> +static RefPtr<DrawTarget> gGlyphDrawTarget;
> +static RefPtr<SourceSurface> gGlyphMask;
> +static RefPtr<SourceSurface> gGlyphAtlas;
> +static Color gGlyphColor;

These global variables don't appear very thread-friendly. I thought we had parallel OMTP now, which (presumably) means there could be multiple paint threads wanting to draw missing glyphs at the same time, which looks like it'd lead to chaos here.

@@ +138,5 @@
> +}
> +
> +static RefPtr<SourceSurface> gWRGlyphAtlas[8];
> +static nsTArray<layers::WebRenderLayerManager*> gWRUsers;
> +static UserDataKey gWRUserDataKey;

I guess it's the global nsTArray here that triggers all the leak reports on your try run. Can we allocate this on first use instead of statically, and then delete it in the Shutdown method?

And again, thread-safety for P-OMTP seems like a concern here.

Comment 25

[Lee Salzman [:lsalzman]]

Attachment: draw missing glyphs from an atlas instead of rectangles

v2, used a LinkedList<WRUserData> instead of an nsTArray to get rid of the leak issue.

Also, regarding P-OMTP, this shouldn't be a problem, as P-OMTP is not going to operate at this level. Rather, it records into a DrawTargetCapture as the target, and then replays that in multiple tiles. This code will only get executed on a single thread.

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 8999997 [details] [diff] [review]
draw missing glyphs from an atlas instead of rectangles

Review of attachment 8999997 [details] [diff] [review]:
-----------------------------------------------------------------

OK, that sounds fair enough.

One bug did catch my eye; aside from that, looks reasonable, but I'd quite like to try it and see how it behaves under stress. Do you expect to get the related WebRender PR merged sometime soon, or should I just try applying it locally for a test build?

::: gfx/thebes/gfxFontMissingGlyphs.cpp
@@ +267,5 @@
> +    uint32_t key = 0;
> +    // Encode orientation in the key.
> +    if (aMat) {
> +        if (aMat->_11 == 0) {
> +            key |= (1 << 4) | (aMat->_12 < 0 ? 1 : 0) | (aMat->_21 < 0 ? 2 : 0);

I think `(1 << 4)` here should just be `4`, otherwise the resulting key may be out of bounds for the gWRGlyphAtlas array.

Comment 27

[Lee Salzman [:lsalzman]]

Attachment: draw missing glyphs from an atlas instead of rectangles

v3, fix 1<<4 bug.

Comment 28

[Lee Salzman [:lsalzman]]

Attachment: colored images for WebRender

This is just a quick patch for WebRender colored image support that you can use to test the mini-font atlas patches, until the upstream WR PR actually gets merged.

Comment 29

[Jonathan Kew [:jfkthame]]

Thanks, that's useful.

I tried the testcase in a local (opt) build, using a locally-saved copy of the testcase, with charset=iso-8859-1 so that it loads with the wrong encoding and shows a tofu glyph as part of each emoji.

It's better, in that the testcase no longer crashes; but the performance is still a big problem -- to the extent that it takes several seconds to render, and the whole browser UI (i.e. the chrome process, not just the tab content) becomes pretty unresponsive while the testcase is open (just moving the mouse around, trying to access a menu, etc., I get frequent cursor beachballing, and extremely slow responses, like several seconds to register a click on a menu).

Here's a profile of loading the testcase.[1] We can see that RenderLayers in the content process is taking 1.8 seconds (ouch!), and we can also see a big lag between this and the compositor finally doing some work -- which I presume is largely because sending all this over IPC takes much too long.

For comparison, if I patch gfxFont::DrawMissingGlyph to just call textDrawer->FoundUnsupportedFeature(), the testcase renders a lot more quickly (profile [2]). It's still not great: there's a noticeable delay in the initial rendering, and trying to select text is terribly sluggish -- like responsiveness is delayed by a second or more -- but it's a lot better than the WR-image version.

(With WebRender disabled altogether, it's better still.[3] The testcase renders snappily, and there's just a bit of UI lagginess, e.g. when drag-selecting in the tofu-sprinkled text. But very usable.)

So while this definitely helps, in that it means we don't crash (though presumably a larger testcase could still break the IPC), I don't think it really gets far enough; we'd still be better off disabling the WR-tofu rendering.

[1] https://perfht.ml/2nGWCMW -- WR enabled, drawing tofu as images
[2] https://perfht.ml/2MsjDB3 -- WR enabled, but tofu flagged as unsupported
[3] https://perfht.ml/2ODwdLm -- WR disabled

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 9000072 [details] [diff] [review]
draw missing glyphs from an atlas instead of rectangles

Review of attachment 9000072 [details] [diff] [review]:
-----------------------------------------------------------------

OK, after discussion on IRC and testing with some less-pathological examples, it looks like the original testcase here is particularly bad because of how the (mis-encoded) text all ends up as one massively-long line, but with a scattering of tofu glyphs -- even a large number of them -- among more "normal" content, we don't see anywhere near such bad perf characteristics.

So let's go with this, to fix the crashing issue (or at least make it substantially harder to reach), and considerably improve perf compared to the current implementation.

Further reducing the IPC overhead in extreme cases (like the testcase here) would be nice if we can do it sometime, but we can leave that as a future enhancement.

::: gfx/thebes/gfxFontMissingGlyphs.cpp
@@ +120,5 @@
> +{
> +    // Get the opaque color, ignoring any transparency which will be handled later.
> +    Color color(aColor.r, aColor.g, aColor.b);
> +    if ((gGlyphAtlas && gGlyphColor == color) ||
> +        MakeGlyphAtlas(color)) {

This would actually be more readable without wrapping the line, IMO.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c610000042a
add support to TextDrawTarget for defining and pushing WR images. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/07dabfefeef1
draw missing glyphs from an atlas instead of rectangles. r=jfkthame

Comment 32

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/9c610000042a
https://hg.mozilla.org/mozilla-central/rev/07dabfefeef1