950000 - Crash [@ nsUserFontSet::CheckFontLoad]

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With:
  user_pref("security.fileuri.strict_origin_policy", false);

About half the time, the testcase crashes [@ nsUserFontSet::CheckFontLoad].

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

I guess the testcase has to be local for the timing to be right to cause the crash.

Comment 3

[Jonathan Kew [:jfkthame]]

The script here does:

  function boom()
  {
    var canvas = document.createElement('canvas');
    var ctx = canvas.getContext('2d');
    ctx.font = 'normal 20px x'; // "x" is a user font family

    document.getElementById("s").remove(); // remove stylesheet that defined font family "x"

    setTimeout(function() {
       ctx.measureText("A");
    }, 50);
  }

At the point where ctx.font is set, there is a user font set in existence, and so the gfxFontGroup that's created and stored in the ContextState of the CanvasRenderingContext2D will have a reference to that font set. Then the stylesheet is removed, which causes the user font set to be deleted - but the font group held by the canvas context's state doesn't know this.

What is the expected behavior here, anyhow - given the script above, what font is the measureText call supposed to use? Should setting a user font as the current font on a canvas context cause it to stick around for subsequent use by that context, even if the rule that defined it is subsequently changed or removed? Or is ctx.font supposed to (in effect) be dynamically resolved each time it's used, so that changes to the user font set are reflected in canvas text behavior without needing to explicitly reset ctx.font?

cc'ing jdaggett and dbaron for any insights. Do you know whether the specs make it clear what's expected here, or is this an ill-defined area?

Comment 4

[Jonathan Kew [:jfkthame]]

Marking security-sensitive, as I suspect this could probably be manipulated from page script to least cause use-after-free errors; not sure what the consequences might be, but best to assume the worst.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: use nsRefPtr for user font set references in fontgroup and prescontext

Possible patch: the idea here is that gfxFontGroup as well as nsPresContext needs to hold a strong reference to the user font set, and when there's an update, the prescontext shouldn't directly call Destroy() on the old set but just release its reference, in case there are live font groups that also have references to it. AFAICT, this resolves the crashes with the testcase here. However, as far as I understand from http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#text-styles, I think our behavior is wrong per spec: if the user font is no longer defined in the style context, then the canvas shouldn't be using it ("if the font style source node does not have that font in scope at the time the font is to be used, then it must be treated as if it was an unknown font, falling back to another"). Fixing that will be a separate matter.

Comment 6

[Jonathan Kew [:jfkthame]]

While the patch above seems to prevent the crash here, it's leading to a large leak reported by Debug Crashtest runs. (Which doesn't entirely surprise me; we're probably ending up with a cycle somewhere that needs to be broken.)

Comment 7

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> What is the expected behavior here, anyhow - given the script above, what
> font is the measureText call supposed to use? Should setting a user font as
> the current font on a canvas context cause it to stick around for subsequent
> use by that context, even if the rule that defined it is subsequently
> changed or removed? Or is ctx.font supposed to (in effect) be dynamically
> resolved each time it's used, so that changes to the user font set are
> reflected in canvas text behavior without needing to explicitly reset
> ctx.font?
> 
> cc'ing jdaggett and dbaron for any insights. Do you know whether the specs
> make it clear what's expected here, or is this an ill-defined area?

While there isn't any explicit text in the HTML5 spec to deal with this specific sequence, I think the effect of the stylesheet deletion should be immediate, anything that depends on it should be rebuilt.  So in this case measureText("A") uses 'font-family: X' and since X is no longer an available font it will end up using the default font.  Same if canvas.style.fontFeatureSettings is changed before the call to measureText, the ContextState needs to be updated and the new ContentState used for measureText.

Comment 8

[Jonathan Kew [:jfkthame]]

I've filed bug 950590 about the incorrect behavior, as that seems like something worth tracking in its own right, separately from the crash here (although fixing it should also resolve the crash, AFAICS).

Comment 9

[Jonathan Kew [:jfkthame]]

This is fixed by bug 950590; the patch here should no longer be needed.

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on FF28, 2013-11-13.
Verified fixed on 24esr, FF27, FF28 and FF29, 2014-01-18.

Comment 11

[Mats Palmgren (inactive)]

Landed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a55b2d51bf

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/70a55b2d51bf