1893303 - hit MOZ_ASSERT(!UpdateResponsiveSource()) in HTMLImageElement::LoadSelectedImage

Status: NEW

(Core :: Layout, defect)

Description

[Ray Kraesig [:rkraesig]]

Visiting https://www.tomshardware.com/how-to/screen-record-in-windows in a debug build of Firefox immediately causes the tab to crash due to this assertion being hit.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

the assertion was added in bug 1694741. Moving the same component as that bug.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Boris, you added the assertion. Could you take a look?

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1694741

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

We'll need a reduced testcase to better analyze the bug.

Comment 5

[Tyson Smith [:tsmith]]

Attachment: testcase.html

The fuzzers found a test case.

Comment 6

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240512212637-45d7400ced7e.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: a74c95816b36cc52e0812c931e4a7cffb5cea14a (20230515094256)
End: 3de3f9428f138bf269718fceb36c5112a195ca37 (20240424093459)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1694741

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Boris, TYlin, does this help?

Comment 9

[Boris Chiou [:boris]]

I can reproduce this assertion with the testcase. Thanks, Tyson.

However, I don't have time to work on this in the current cycle. Please feel free to take this.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Yes, the fuzzer testcase works!

Comment 11

[Bugmon [:jkratzer for issues]]

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Comment 12

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 13

[Daniel Holbert [:dholbert]]

Looking in pernosco: when the assertion calls UpdateResponsiveSource and expects no-change, we do in fact change the selected source from 0 to -1 (i.e. no-image-at-all), because our document has a null PresShell/PresContext pointer at that moment, which makes us bail out here:
https://searchfox.org/mozilla-central/rev/445a6e86233c733c5557ef44e1d33444adaddefc/dom/base/ResponsiveImageSelector.cpp#349-350

if (!pctx || !baseURI) {
  return oldBest != -1;

(This is return true because oldBest was 0, because it was established at an earlier time when our PresShell was non-null.)

As to why our PresShell/PresContext is null, I'm guessing it's due to the document.designMode = "on" in the testcase which probably causes some presentation stuff to be reinitialized.

Comment 14

[Daniel Holbert [:dholbert]]

I tried skipping the assertion if we have a null prescontext, and that fixes the fuzzer testcase, but doesn't fix the issue at Toms Hardware -- we still trip the assertion there. So something else is going on there...

Comment 15

[Tyson Smith [:tsmith]]

Site-scout is also reporting this fairly regularly, would you like me to try to get a Pernosco session?

Comment 16

[Daniel Holbert [:dholbert]]

I just recorded & submitted one for Tom's Hardware; I'll post the link here when I've got it.

Comment 17

[Daniel Holbert [:dholbert]]

Darn, my Toms Hardware rr recording had issues apparently. I'm now booted into Windows & and can't reboot/re-record at the moment, but I'll do that later unless someone gets to it first.

Comment 18

[Tyson Smith [:tsmith]]

A Pernosco session (Tom's Hardware) is available here: https://pernos.co/debug/A11ZkUOrMngQOb_fjYhi_g/index.html

Comment 19

[Daniel Holbert [:dholbert]]

Thanks. So in that one, UpdateResponsiveSource is returning true because mResponsiveSelector is ending up non-null (though it's null at the start of the function); and we reach the end of the function which has this return statement:
https://searchfox.org/mozilla-central/rev/a18a7c526cf3c531f2fc24db4f0dffbc16290a7e/dom/html/HTMLImageElement.cpp#1098

return hadSelector || mResponsiveSelector;

...which returns true because mResponsiveSelector is non-null.

(I don't know this code well enough to follow precisely what's going on or which part is unexpected, though.)