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

NEW
Unassigned

Status

()

Core
ImageLib
P5
minor
4 months ago
a month ago

People

(Reporter: mats, Unassigned)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
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)".))
(Reporter)

Comment 1

4 months ago
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...
(Reporter)

Comment 2

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d40665117d9976976ca8496b2a1d6d63462af54
(Reporter)

Comment 3

4 months ago
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?
(Reporter)

Comment 5

4 months ago
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).
(Reporter)

Comment 6

4 months ago
(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...
Whiteboard: [gfx-noted]
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.