913247 - Don't call RequestRefresh from a hashtable enumerator

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

As bz noted in bug 911862 comment 17, we call RequestRefresh from a hashtable enumerator (nsRefreshDriver::ImageRequestEnumerator ), and we can't do that anymore, now that RequestRefresh can trigger script.

Filing this bug on making us gather up all the images from the hashtable (e.g. into a local array) in one pass, and then doing a second pass across that array to perform the RequestRefresh calls.

(This blocks bug 911862, because we shouldn't ship the fix for bug 911862 without also shipping this fix.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

I think this should do it.

(tagging bz for feedback in case he has any thoughts; tagging jwatt for review, since he volunteered in IRC and since bz's already expressed support for the general idea of this patch over in bug 911862 comment 17.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1a (now initializing array to the right capacity)

This version includes an optimization I'd forgotten to use in the first version -- we know the approximate size of the temporary array that we need, up-front, so we can just ask for that size right away rather than going through the default doubling array-allocation strategy.

Comment 3

[Daniel Holbert [:dholbert]]

(aside: I'm also happy to change the temporary array to be nsTArray<nsCOMPtr<imgIContainer> >, too. I'm not clear on what the tradeoff is between that vs. what I'm using now, nsCOMArray<imgIContainer>.  I asked in IRC if there were any reasons to use one vs. the other, and tbsaunde says "nsCOMArray may have slightly smaller code size otherwise nothing really [differentiates them]".  It looks like nsCOMArray has some extra functions that I don't need, which made me think about switching to nsTArray in case there was overhead associated with those; but if it's possibly-more-compact then there's probably no reason to switch.)

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (aside: I'm also happy to change the temporary array to be
> nsTArray<nsCOMPtr<imgIContainer> >, too. I'm not clear on what the tradeoff
> is between that vs. what I'm using now, nsCOMArray<imgIContainer>.  I asked
> in IRC if there were any reasons to use one vs. the other, and tbsaunde says
> "nsCOMArray may have slightly smaller code size otherwise nothing really
> [differentiates them]".  It looks like nsCOMArray has some extra functions
> that I don't need, which made me think about switching to nsTArray in case
> there was overhead associated with those; but if it's possibly-more-compact
> then there's probably no reason to switch.)

Neil (Rashbrook) claimed on IRC recently that nsCOMArray creates less code bloat and is slightly faster (I looked quite some time ago and remember concluding that was the case too). khuey on the other hand seems to prefer nsTArray<nsCOMPtr<T> >. I don't know the reasons for khuey's preference, but I find it annoying that nsCOMArray has legacy methods duplicating the nsTArray compatible methods that were added to it, but with differences such as the param order being different, and "Object" vs. "Element" is something I have to think about and occasionally get wrong and have to recompile for.

Anyway, I don't really think it's that big a deal so I'm happy either way. If anyone feels strongly they can file a follow-up.

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 800489 [details] [diff] [review]
fix v1a (now initializing array to the right capacity)

Looks good to me.

Comment 6

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/547d6f20ecb2

Given the time pressure to get the fix for bug 911862 into aurora I took the liberty of pushing this after tweaking the comment a bit to make the issue clearer.

Comment 7

[Daniel Holbert [:dholbert]]

Thanks!

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/547d6f20ecb2

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 800489 [details] [diff] [review]
fix v1a (now initializing array to the right capacity)

Requesting approval as this is a tweak to the patch for bug 911862 which was a patch split out from bug 907503. See bug 907503 comment 43 for the main approval request.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 800489 [details] [diff] [review]
fix v1a (now initializing array to the right capacity)

Looks good, though the manual Clear() call is superfluous.

Comment 11

[Daniel Holbert [:dholbert]]

Oh, true; that was there because in an earlier (local) version of the patch, I declared the array outside of that scope, and I wanted to make it clear that we weren't going to use the array's contents after that point.

I'll push a followup to remove it, with rs=bz granted over IRC.  (We can merge that in with the fix v1a when it lands on aurora; or if we forget, it doesn't really matter, since the followup won't affect behavior at all.)

Comment 12

[Daniel Holbert [:dholbert]]

Landed followup to remove the superfluous Clear() call:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/94e1942ba378

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 800489 [details] [diff] [review]
fix v1a (now initializing array to the right capacity)

Approving for Aurora uplift (and will track since it's connected to bug 907503) - I assume what you uplift will contain the follow up that dholbert pushed hours ago.

Comment 14

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/03684770098a

(In reply to [e-mail] [:lsblakk] from comment #13)
> I assume what you uplift will contain the follow up that dholbert
> pushed hours ago.

Yes, I rolled that into this changeset, although it wouldn't really matter.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94e1942ba378

Comment 16

[u279076]

Does this require any testing to verify it's working correctly?

Comment 17

[Daniel Holbert [:dholbert]]

No, I don't think so.

Comment 18

[u279076]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> No, I don't think so.

Thanks Daniel, flagging as [qa-].