591832 - Crash [@ ImageRenderer::PrepareImage] on printing/print preview with -moz-element

Status: RESOLVED WORKSFORME

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build when printing/print preview, when you have the option enabled for printing backgrounds (under Page Setup).

http://crash-stats.mozilla.com/report/index/1d998b26-585d-43aa-a18d-ae3d32100829
0  	xul.dll  	ImageRenderer::PrepareImage  	
1 		@0xda14267 	
2 	xul.dll 	nsCSSRendering::PaintBackgroundWithSC 	layout/base/nsCSSRendering.cpp:2373

Comment 1

[Joe Drew (not getting mail)]

Marcus, can you look at this?

Comment 2

[Daniel Veditz [:dveditz]]

Was the 1.9.1 blocking nomination a mistake? According to crash-stats this has only been seen on the trunk.

Comment 3

[Joe Drew (not getting mail)]

Must have been, because this code doesn't exist on any branch, only on the trunk.

Comment 4

[Markus Stange [:mstange]]

Attachment: nullcheck mForFrame->GetContent()

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm confused. How can mForFrame be non-null but its content be null? What kind of frame is it? I thought we wouldn't find a frame in this testcase.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> How can mForFrame be non-null but its content be null?

If mForFrame is an nsViewportFrame.

Comment 7

[Markus Stange [:mstange]]

Right, it was an nsViewportFrame. Is there another quick way of getting that frame's document URI?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, how could we be getting a viewport frame here? ID #a doesn't match any element, so shouldn't we get no frame at all?

Comment 9

[Boris Zbarsky [:bzbarsky]]

The frame in question is the one we're trying to resolve #a relative to.  We're using it to get the base URI.

  mForFrame->PresContext()->GetDocument()->GetBaseURI()

should do the trick for the viewport, right?  That said, this code looks fishy to me in general.  Why are we using (base) URIs at all here, if we know we have an ID?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Because nsSVGEffects::GetPaintingPropertyForURI is shared code that depends on a URI.

I guess this makes sense. If mForFrame->GetContent() is null we should just use the document.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, but why does using the base URI of the element make sense?  Or does that URI end up being compared to the document base URI?  And if so, won't this get things wrong when the element's base URI is different from the document's?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good point. I guess we should just always use the document URI here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

You mean document's base URI?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I actually mean the document URI. We want to avoid the external resource path in nsReferencedElement::Reset.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, that compares to the document URI, not the document base URI?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That seems somewhat wrong, unless the relevant callsites also use the document URI...

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah OK. The question is whether an element with ID #id should be accessible via a reference to the document URI or the document base URI. The base URI kinda makes sense because then url(#xyz) always refers to the current document. But then if the document URI is doc.html and the base URI changes to base.html, a reference url(doc.html#xyz) has to be reresolved from the current document to an external resource document, and a reference url(base.html#xyz) has to be reresolved from an external resource document to the current document. I guess that's doable without major API changes, since nsReferencedElement can handle the reresolving. It's a pain, but everything to do with <base> is a pain...

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oh, and isn't there stuff in the HTML5 spec about URI resolution happening at set times, so that a URI already resolved from #xyz relative to the current document URI would keep the same URI during a base change, so we'd actually have to load the current document URI as an external resource document and start referring to the non-live element there? That would be ultra confusing for authors! And we still have to reresolve url(doc.html#xyz) and url(base.html#xyz)! What a mess!

Hmm, but when we load doc.html into an external resource document, if it has a <base> element do references to elements via doc.html#xyz break or something? Arrrgh!

Altogether I think it would be better to say that an element with ID #id is accessible via a reference to the document URI, and NOT the document base URI. All of those problems go away, except for the problem that a reference url(#xyz) can break if your document uses <base>. "Don't Do That."

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, I see what you mean.  Hmm.  I think you're right; per SVG, #foo just resolves relative to the base, and if the result is not in your document then you need to load a resource.  So yeah, comparing to the document URI is the right thing.  And I agree that for -moz-element we should then use the document URI, always.

Comment 21

[Robert Kaiser]

This can still be reproduced in Nightly.

Comment 22

[Martijn Wargers (dead)]

Still happening.

Comment 23

[Andrei Purice]

This issue is still reproducible for me on the latest versions of Firefox Nightly 87.0a1 (2021-02-16), beta 86.0 and release 85.0.2 on Windows 7x86.
Updating the flags accordingly.

Comment 24

[Julien Cristau [:jcristau] (back April 22)]

I guess Markus isn't still working on this after 10 years.