Closed Bug 1433351 Opened 2 years ago Closed 2 years ago

Intermittent image/test/reftest/pngsuite-ancillary/cs3n2c16.png | application timed out after 370 seconds with no output after Assertion failure: mExistingEntry || mDidInitNewEntry (Forgot to call OrInsert() on a new entry)

Categories

(Core :: Graphics: WebRender, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Guessing based on the stack where this belongs.
Component: ImageLib → Graphics: WebRender
This is caused by a combination of two issues:

1) We refuse to allocate a new TextureClient if the surface size is larger than the max texture size. With WebRender we really ought to not to refuse to allocate the surface. We don't encounter this is normal use with WebRender because probably are using shared surfaces which do not use this code path. This will be mostly fixed as part of bug 1435291, which will enable shared surfaces by default, and lift the max texture size restriction for image containers with WR (does not apply since it can tile).

2) We abuse the LookupForAdd method, since we might not actually add an entry if the texture allocation fails. EntryPtr should have an "OrCancel" method to let us bail. We should fix that here.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Attachment #8958577 - Attachment is obsolete: true
Attachment #8958791 - Flags: review?(nfroyd)
Comment on attachment 8958791 [details] [diff] [review]
0001-Bug-1433351-Add-nsBaseHashtable-EntryPtr-OrRemove-me.patch, v2

Review of attachment 8958791 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsBaseHashtable.h
@@ +310,5 @@
>        }
> +      return mEntry->mData;
> +    }
> +
> +    void OrRemove()

I think this is pretty reasonable.  I am a little concerned that EntryPtr now has a profusion of states, e.g. after OrRemove(), we now have mExistingEntry == true, but no actual mEntry.  I guess the asserts will catch that, though, and ideally nobody will write:

  entry.OrRemove(...);
  entry.OrInsert(...);

I don't think we have to get the API completely right in this bug, but if you have better ideas to solve this than the status quo, I am interested.

::: xpcom/tests/gtest/TestHashtables.cpp
@@ +477,5 @@
> +
> +  // Remove newly added entries via OrRemove.
> +  for (auto& entity : gEntities) {
> +    auto entry = UniToEntity.LookupForAdd(entity.mUnicode);
> +    ASSERT_FALSE(entry);

Tests!  <3
Attachment #8958791 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #7)
> Comment on attachment 8958791 [details] [diff] [review]
> 0001-Bug-1433351-Add-nsBaseHashtable-EntryPtr-OrRemove-me.patch, v2
> 
> Review of attachment 8958791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/ds/nsBaseHashtable.h
> @@ +310,5 @@
> >        }
> > +      return mEntry->mData;
> > +    }
> > +
> > +    void OrRemove()
> 
> I think this is pretty reasonable.  I am a little concerned that EntryPtr
> now has a profusion of states, e.g. after OrRemove(), we now have
> mExistingEntry == true, but no actual mEntry.  I guess the asserts will
> catch that, though, and ideally nobody will write:
> 
>   entry.OrRemove(...);
>   entry.OrInsert(...);
> 
> I don't think we have to get the API completely right in this bug, but if
> you have better ideas to solve this than the status quo, I am interested.
> 

None at the moment, but I think on it a bit.
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f16d20b0a3b
Add nsBaseHashtable::EntryPtr::OrRemove method to abort nsBaseHashtable::LookupForAdd on miss. r=froydnj
Backout by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a4c7d982b2
Backed out changeset 8f16d20b0a3b for build bustage on a CLOSED TREE.
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47ea5c26a80
Add nsBaseHashtable::EntryPtr::OrRemove method to abort nsBaseHashtable::LookupForAdd on miss. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/b47ea5c26a80
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.