585129 - mIsActive should be propagated to resource document presshells

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Bobby Holley (:bholley)]

In bug 343515 (landed), we don't propagate mIsActive to resource document presshells. This is a problem, because it means that resource documents will default to active and hold locks on all of their images once bug 512260 lands.

bz pointed this out in bug 512260 comment 13.

I can't tell how important this is, because I'm not sure how common it is to have a lot of images in resource documents. Nominating for blocking, hoping that the driver will know.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think this is a huge deal given the current content on the Web, but it's pretty important to stuff that we want to promote (e.g., I could certainly see SVG filters pointing to images via svg:image elements, which I presume are relevant here), and it seems like it should be pretty straightforward to fix, so I'm inclined to go for blocking here.  That said, if it's not straightforward to fix, I'd reconsider.

Comment 2

[Bobby Holley (:bholley)]

(In reply to comment #1)
> I don't think this is a huge deal given the current content on the Web, but
> it's pretty important to stuff that we want to promote (e.g., I could certainly
> see SVG filters pointing to images via svg:image elements, which I presume are
> relevant here), and it seems like it should be pretty straightforward to fix,
> so I'm inclined to go for blocking here.  That said, if it's not
> straightforward to fix, I'd reconsider.

Sounds good. I'll put this at the bottom of my blocker list for now, since if things get tight it would be better for me to spend time on my imagelib blockers and have someone with more content/layout fu do this. But I think I should be able to get to it.

Comment 3

[Bobby Holley (:bholley)]

I don't have the cycles to learn the relevant code to fix this. Resetting assignee to default.

It should be very straightforward for someone who knows how resource documents interact with their parents. Given that we're also using mIsActive for other things now (invalidation suppression, animation control), I think it'd be a good idea to fix this for ff4. Maybe dholbert can do it?

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Maybe dholbert can do it?

Hopefully in the betaN timeframe.  (can't think about it too much right now because I'm focusing on SVG-as-image followups.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

bz, is this what you had in mind?

FWIW, I'm running this through TryServer right now as a sanity-check, and it's already passed all tests on Linux Opt.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488322 [details] [diff] [review]
fix v1

Hmm.  This looks ok, but we need to fix QueryIsActive too, to work for resource docs, right?  I'd assume those have a null container on the prescontext....

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v2: fix QueryIsActive too

Ok -- this version tweaks QueryIsActive to check the display document's container / docshell, for external resource documents.

> I'd assume those have a null container on the prescontext....

Patch includes a NS_ABORT_IF_FALSE to explicitly state this assumption.

Passed crashtests & SVG reftests locally.  Running through try-server for sanity-check, & then will request review.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, that looks good.  I'd s/Ext/External/g, though.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v3, with s/ext/external/g

Comment 10

[Daniel Holbert [:dholbert]]

(Comment 7 passed tests on TryServer btw)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488712 [details] [diff] [review]
fix v3, with s/ext/external/g

r=me

Comment 12

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/af0f637bc602