1643635 - The browser crashes when document.fonts.values() is used as a WeakMap key

Status: VERIFIED FIXED

(Core :: DOM: Bindings (WebIDL), defect)

Description

[Jon Coppeard (:jonco)]

If you type the following into the console:

  new WeakMap().set(document.fonts.values(), undefined);

This assertion fails: https://searchfox.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#1147

Comment 1

[Jon Coppeard (:jonco)]

Crash report: https://crash-stats.mozilla.org/report/index/6ca6ff07-dd4d-4da9-b06d-eb9850200605

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Isn't the assertion just too strict.
https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/layout/style/FontFaceSetIterator.h#17,21
https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/dom/bindings/Bindings.conf#298
The object is cycle collectable, but only JS can ever own it.
Similar case to when nsISupports object isn't wrappercached
https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/dom/canvas/ImageData.h#25,34-35
https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/dom/bindings/BindingUtils.cpp#1121,1128-1129,1133

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: Bug 1643635, it is ok to have a cycle collected non-nsISupports class which doesn't inherit nsWrapperCache, r=mccr8!,peterv!

The method lets one to have nsISupports objects not supporting nsWrapperCache, but non-nsISupports objects are
required to inherit nsWrapperCache because of the assertion.

jonco is adding tests in https://bugzilla.mozilla.org/show_bug.cgi?id=1642974

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1351501#c11 requested the assertion, and the patch should still check that
"We should also assert that addProperty is non-null if and only if domClass->mParticipant is non-null, right?"

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/431b35a5b06c
it is ok to have a cycle collected non-nsISupports class which doesn't inherit nsWrapperCache, r=mccr8,peterv

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/431b35a5b06c

Comment 8

[Oana Botisan, Desktop QA]

I can't seem to be able to reproduce this issue. Can you provide me with more specific steps or could you please check if the issue is reproducing on the Firedox 79.0b2?

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Oana Botisan, Desktop Release QA from comment #8)
It's fixed on 79 but it reproduces on 78.

Comment 10

[Oana Botisan, Desktop QA]

Thank you, Jon, for verifying the fix. That is fine because only 79 was fixed and the milestone is firefox79. I will mark this bug as verified fixed according to comment 9.