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)".))
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...
Created attachment 8882335 [details] [diff] [review] 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
Maybe GetFirstFrameDelay(aRequest) changes from when we put the request in the hashtable and when we remove it?
Created attachment 8882343 [details] [diff] [review] 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).
(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...
2 years ago
Priority: -- → P3
status-firefox57: --- → fix-optional
Priority: P3 → P5
You need to log in before you can comment on or make changes to this bug.