don't panic when an invalid font instance key is supplied

[lsalzman]

This is a small modification to my previous FontInstanceKey to allow invalid font instance keys to work around the issue described in https://bugzilla.mozilla.org/show_bug.cgi?id=1396056

There are somewhat legitimate situations where this can occur, so we don't want to panic.

---

This change is 

[jrmuizel]

Can you describe those situations?

[glennw]

This seems reasonable to me - I think the API should probably not panic if an invalid key is supplied. Are you fine with merging this @jrmuizel ? We might like to consider having a "strict" mode in the future, where invalid API calls to panic?

[lsalzman]

Gankro at least found one case where we invalidate the whole font cache on the Gecko side while a display item using some of the fonts therein is in flight. Now, arguably, that could be looked at as a bug on its own, but I would rather just have us fail to render and warn in those situations rather than simply crash, since we will probably encounter similar bugs in the future in releases.

[jrmuizel]

I'd rather we crash so we catch these bugs earlier. If we need to turn off this panic than I'd rather it be because we're forced into it. This is basically a use after free and I'd like to better understand why we need that to be valid before giving into turning this into a warning.

[Gankra]

I think this is a reasonable strategy. For the specific case, it means updating fonts can cause text to flicker for a frame; not a big deal. More broadly "not rendering" is a fairly reasonable solution to "no font", and if we ever have any serious bugs where we're losing fonts, the warnings should be fairly clear for those debugging it.

[glennw]

We recently changed the render call to return a Result which contains a list of errors that occurred while rendering the frame.

This is used to gracefully handle the situation where a driver fails to compile a normally valid shader (since they are compiled lazily on first use). See

webrender/webrender/src/renderer.rs

Line 1747 in 400ae87

pub fn render(&mut self, framebuffer_size: DeviceUintSize) -> Result<(), Vec<RendererError>> {

for details.

My suggestion is to merge this PR, and as a follow up I'll add the infrastructure to enable passing errors like this through the pipeline to be returned as part of the list of errors that occurred during rendering that frame.

If we do that, this will mean:

• WR itself will emit a warning but not panic.
• Client code can read the error list and choose what to do based on the error(s). For example, panic in a CI / debug build and log a warning in a production build.

@jrmuizel Does that sound reasonable?

[kvark]

@glennw plan sounds reasonable to me.
I'd also like to be sure that logging from warn! is configured to be visible (either on screen or via some Gecko-specific delivery mechanisms) by Firefox.

[glennw]

Added #1677 and #1678

@bors-servo r+

[bors-servo]

📌 Commit f2ae2ce has been approved by glennw

[bors-servo]

⌛ Testing commit f2ae2ce with merge 0d360c1...

[bors-servo]

☀️ Test successful - status-travis
Approved by: glennw
Pushing 0d360c1 to master...