Closed Bug 1466723 Opened 2 years ago Closed 2 years ago

Fix race in dom/base/test/test_data_uri.html

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

https://dxr.mozilla.org/mozilla-central/rev/ad1249c83efb4c01bb86a1f4e4744af73d10acf8/dom/base/test/test_data_uri.html#148

contains code along the lines of:

window.onload = function()
{
  let p7 = new Promise(resolve => {
    let text = document.createElement('p');
    text.style = 'font-family: DataFont';
    text.innerHTML = "This text should trigger 'TestFont' to load.";
    document.body.appendChild(text);
    document.fonts.onloadingdone = function (fontFaceSetEvent) {
      is(fontFaceSetEvent.fontfaces.length, 1);
      resolve();
    };
  });
}

In this scenario we can expect document.fonts.onloadingdone to fire one or two times. It can fire for the initial load (in which case fontFaceSetEvent.fontfaces.length==1). Then after the onload handler inserts the 'p' element into the DOM we load the font TestFont, and once that is loaded 'loadingdone' will fire again (this time fontFaceSetEvent.fontfaces.length==1).

If the first event hasn't dispatched before appendChild() is called, there will be only one event (the latter).

This test assumes that either only one instance of the event is fired (the latter), or that if the first instance is fired that it will have been processed by the time that the 'load' event is fired (nothing guarantees this).

If we end up firing two events, we fire the first (asynchronously) at some point after the DOMContentLoaded event has fired, generally under nsRefreshDriver::Tick. The document's 'load' event can then end up being fired (synchronously) while the 'loadingdone' event is still waiting to be dispatched. If this happens, then the onload handler sets the document.fonts.onloadingdone handler, which then triggers for the first 'loadingdone', resulting in the test failing.
Basically whenever script makes a change that will result in the font face set going from loaded, to loading new fonts, there is no guarantee that the next 'loadingdone' event will be for the loading of the new fonts. It could in fact be for a previous loading done state. So I don't think using that event in tests (that aren't specifically about testing that event) is a good idea. It requires a lot more careful construction to be robust than using document.fonts.ready.

Much more robust is to have script make its changes, then listen for the document.fonts.ready Promise to resolve. Since the Promise will be replaced immediately on any changes that require it, script can then be sure when its listener triggers it is for the right font face set changes.
Attached patch patchSplinter Review
Attachment #8983270 - Flags: review?(cam)
(In reply to Jonathan Watt [:jwatt] from comment #0)
> It can fire for the initial load (in which case
> fontFaceSetEvent.fontfaces.length==1).

Ugh. I meant ==0 in this case of course.
Comment on attachment 8983270 [details] [diff] [review]
patch

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

Your reasoning makes sense.  An alternative for this sub-test, which might be simpler, could be to just create a FontFace object with the data: URL, call load() on it, and check if the Promise returned is resolved or rejected.  But this is fine as is.
Attachment #8983270 - Flags: review?(cam) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d845389b97
Fix race in dom/base/test/test_data_uri.html. r=heycam
Blocks: 1465983
https://hg.mozilla.org/mozilla-central/rev/46d845389b97
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.