Closed Bug 913247 Opened 6 years ago Closed 6 years ago

Don't call RequestRefresh from a hashtable enumerator

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 + fixed
firefox26 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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.)
Attached patch fix v1 (obsolete) — Splinter Review
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.)
Attachment #800486 - Flags: review?(jwatt)
Attachment #800486 - Flags: feedback?(bzbarsky)
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.
Attachment #800486 - Attachment is obsolete: true
Attachment #800486 - Flags: review?(jwatt)
Attachment #800486 - Flags: feedback?(bzbarsky)
Attachment #800489 - Flags: review?(jwatt)
Attachment #800489 - Flags: feedback?
(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.)
(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 on attachment 800489 [details] [diff] [review]
fix v1a (now initializing array to the right capacity)

Looks good to me.
Attachment #800489 - Flags: review?(jwatt) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/547d6f20ecb2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Attachment #800489 - Flags: approval-mozilla-aurora?
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.
Attachment #800489 - Flags: feedback?(bzbarsky) → feedback+
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 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.
Attachment #800489 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/03684770098a

(In reply to lsblakk@mozilla.com [: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.
Does this require any testing to verify it's working correctly?
Flags: needinfo?(dholbert)
No, I don't think so.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #17)
> No, I don't think so.

Thanks Daniel, flagging as [qa-].
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.