1144450 - infinite recursion when inspecting document.fonts.status from loadingdone event listener

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In the extended test in bug 1086207, we get:

  JavaScript error: file:///tmp/test.html, line 53: too much recursion

This happens due to accessing document.fonts.status from within the loadeddone event listener.

Comment 1

[Cameron McCormack (:heycam)]

I think the problem is that in FontFaceSet::UpdateRules, we do not have the same "check to see that the FontFaces we're adding already existed in the same order" for non-rule FontFaces that we do for rule FontFaces.  This means every time we call UpdateRules when there is a non-rule FontFace present, we end up thinking that there was a change the font face set, which means calling CheckLoadingStarted.

CheckLoadingStarted also has a problem (I think) that it replaces mReady with a new promise (and sets mReadyIsResolved to false, which is the cause of the infinite recursion) even when HasLoadingFontFaces() return false.  We should only be replacing mReady at the same time as setting mStatus to Loading and dispatching the loading event.

Comment 2

[Cameron McCormack (:heycam)]

In this bug I will fix the latter problem.  Fixing the former will then become an optimization to save a bit of work.

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=119b34bcecd7

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 1: Fix a typo in test_font_loading_api.html.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 2: Don't replace a FontFaceSet's ready promise when there are no loading FontFaces.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 3: Replace uses of mDispatchedLoadingEvent with mStatus, as they're equivalent.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 4: Replace uses of mReadyIsResolved with mStatus, as they're equivalent.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 5: Fix/add tests for not firing loadingdone/loadingerror when adding an already-loaded/error FontFace to a FontFaceSet.

Comment 9

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92b64d73900c

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a106024f6b0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c86fe2f8e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8530a96751c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/fabd2ae32692
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a83dc07c6c8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a106024f6b0d
https://hg.mozilla.org/mozilla-central/rev/f2c86fe2f8e9
https://hg.mozilla.org/mozilla-central/rev/8530a96751c6
https://hg.mozilla.org/mozilla-central/rev/fabd2ae32692
https://hg.mozilla.org/mozilla-central/rev/6a83dc07c6c8