1377142 - Investigate why the implied invariant in nsRefreshDriver::RemoveImageRequest does not hold

Status: NEW

(Core :: Graphics: ImageLib, enhancement, P5)

Description

[Mats Palmgren (inactive)]

http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/layout/base/nsRefreshDriver.cpp#1342-1343

  // Try to remove from both places, just in case, because we can't tell
  // whether RemoveEntry() succeeds.

That seems to imply that aRequest should not be in both the mRequests
and mStartTable at the same time.  However, this invariant doesn't hold,
as this Try run shows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=287dbd181e2e82e5625e8816b926483605fd406d
https://hg.mozilla.org/try/rev/74458487863edc26b7490894ea9cb56353bee220

If it's possible to enforce this invariant, then we could avoid some
hashtable lookups here.

(BTW, the "we can't tell whether RemoveEntry() succeeds" is a bald-faced lie.
nsTHashtable has always had that capability: just do GetEntry followed by
a RemoveEntry.  (And nowadays we also have "bool EnsureRemoved(key)".))

Comment 1

[Mats Palmgren (inactive)]

Ha, now I see that my assertion there was actually wrong:

+    MOZ_ASSERT(!mStartTable.Contains(GetFirstFrameDelay(aRequest)),
+               "shouldn't be in both tables");

It should be something like:

+    MOZ_ASSERT(!mStartTable.Contains(GetFirstFrameDelay(aRequest)) ||
+               !mStartTable.Get(GetFirstFrameDelay(aRequest))->mEntries.Contains(aRequest),
+               "shouldn't be in both tables");

I'll make a Try run to see if it holds after all...

Comment 2

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d40665117d9976976ca8496b2a1d6d63462af54

Comment 3

[Mats Palmgren (inactive)]

Attachment: WIP part 1

Nope, still doesn't hold:
browser/components/extensions/test/browser/browser_ext_getViews.js
Assertion failure: !IsInStartTable(aRequest) (shouldn't also be in mStartTable), at /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1349

Comment 4

[Timothy Nikkel (:tnikkel)]

Maybe GetFirstFrameDelay(aRequest) changes from when we put the request in the hashtable and when we remove it?

Comment 5

[Mats Palmgren (inactive)]

Attachment: WIP part 2

GetFirstFrameDelay does some ref-counting and a virtual call,
so I figured this would be an optimization to avoid that in
the case mStartTable is empty, which I assume is the common
case (no animated images).

Comment 6

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> Maybe GetFirstFrameDelay(aRequest) changes from when we put the request in
> the hashtable and when we remove it?

Yeah, that's likely what happens.  This setup is a bit fragile...