Closed
Bug 1433351
Opened 6 years ago
Closed 6 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)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla61
| Tracking | Status | |
|---|---|---|
| firefox61 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Assigned: aosmond)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 1 obsolete file)
|
6.24 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Filed by: philringnalda [at] gmail.com https://treeherder.mozilla.org/logviewer.html#?job_id=158496054&repo=mozilla-inbound https://queue.taskcluster.net/v1/task/CGVv_P7oTtqAXZKwShU9Fw/runs/0/artifacts/public/logs/live_backing.log https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/CGVv_P7oTtqAXZKwShU9Fw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
|
||
Comment 1•6 years ago
|
||
Guessing based on the stack where this belongs.
Component: ImageLib → Graphics: WebRender
| Comment hidden (Intermittent Failures Robot) |
|
||
Updated•6 years ago
|
Blocks: stage-wr-year
| Comment hidden (Intermittent Failures Robot) |
|
Assignee | |
Comment 4•6 years ago
|
||
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
|
|
Assignee | |
Comment 5•6 years ago
|
||
|
|
Assignee | |
Comment 6•6 years ago
|
||
Attachment #8958577 -
Attachment is obsolete: true
Attachment #8958791 -
Flags: review?(nfroyd)
|
||
Comment 7•6 years ago
|
||
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+
|
|
Assignee | |
Comment 8•6 years ago
|
||
(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
|
||
Comment 10•6 years ago
|
||
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.
|
|
||
Comment 11•6 years ago
|
||
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
|
||
Comment 12•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/b47ea5c26a80
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•