Closed Bug 1663865 Opened 4 years ago Closed 4 years ago

MOZ_CRASH(Non-tiled image with no visible images detected! Properties None) at gfx/wr/webrender/src/batch.rs:2300

Categories

(Core :: Graphics: WebRender, defect)

defect

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: tnikkel, Assigned: gw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

With my patch for bug 1663562 we hit this pretty consistently in the dom/base/test/test_meta_viewport* tests.

Example log
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315175246&repo=try&lineNumber=5233

It seems easy to reproduce:

  1. apply https://hg.mozilla.org/integration/autoland/rev/6698b842822e
  2. make a linux debug build
  3. ./mach mochitest dom/base/test/ -f plain --start-at dom/base/test/test_location_href_unknown_protocol.html --enable-webrender

You should crash pretty quickly.

The applied patch just potentially adds an extra reflow, it doesn't seem like it should affect webrender.

One weird thing that the tests do is change the pref layout.css.devPixelsPerPx, which depending on your screen can make the browser ui change sizes.

This blocks desktop zooming work, which we are hoping to enable in 82.

Glenn, could you get someone to take a look and see if the problem is in webrender or bad data being passed into webrender?

Flags: needinfo?(gwatson)

Dzmitry, looks like this panic came from your patch, but it seems unrelated to that change. Nical, any ideas what might trigger this?

Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(gwatson)
Flags: needinfo?(dmalyshau)

I recall talking about this with you, Glenn, quickly at some point. I asked if it would make sense to turn the visible_tiles into some sort of an option or an enum, so that we don't mistakenly check for empty and think that it's non-tiled. But I think there was a convincing argument that this will never happen. In particular, I saw that the associated ImageProperties knows about this aspect unconditionally, and in most places we have it by hand already, so matching another enum looked iffy. So I just ended up adding an assert there. I'm glad I did!

So the assert just revealed a bug in the original logic that was obscured. I believe this code is fine, but we need to investigate the repro case, and fix it in a way that a tiled image with no visible tiles never reaches this code in the first place. Shouldn't be hard now that we have the repro!

Flags: needinfo?(dmalyshau)
Severity: -- → S3

I guess this should be prioritized higher if this is blocking desktop zooming for 82.

Severity: S3 → S2

Oddly, I ran the tests from comment 0 multiple times and didn't hit the assertion.

From what I know of the code, an empty list of visible tiles for a tiled image means the primitive should have been culled out (prim_instance.visibility_info = PrimitiveVisibilityIndex::INVALID) and the batching code shouldn't have seen the primitive. Or the image's visibility should have been updated earlier this frame and somehow it wasn't.

Short of being able to reproduce I looked at the code for a bit and it's hard to tell what is going wrong. A few notes:

  • Whether or not an image is tiled can only ever change when creating or updating the image which always happens outside of frame building (so the tiled-ness can't change between the visibility pass and batching).
  • A primitive that isn't marked as visible should not get to the batching code in that crash stack, see https://searchfox.org/mozilla-central/rev/0c97a6410ff018c22e65a0cbe4e5f2ca4581b22e/gfx/wr/webrender/src/batch.rs#772
  • Assuming update_primitive_visibility does run over all primitives, the first thing it does for each primitive is call reset() on it which marks it as invisible and then actually determines whether it is visible and updates the visibility info accordingly. In principle that should guard us against buggy control flow bailing out before setting the visibility info.
  • For most primitive kinds, the point where we actually mark a primitive as visible is here: https://searchfox.org/mozilla-central/rev/0c97a6410ff018c22e65a0cbe4e5f2ca4581b22e/gfx/wr/webrender/src/batch.rs#772 just before calling request_resources_for_prim which is where we reset the visibility info back to invisible if we find that there is no visible tile. The code there is rather straightforward, it doesn't look like we can skip checking the size of the visible tiles array when we get to this point.

Perhaps storing the framestamp of the last frame where a primitive was visible in the visibility pass and comparing against the current frame to determine if the primitive is currently visible in later passes would be more robust, since it would avoid processing primitives that we failed to visit during the visibility pass (if that's possible). That would be at the cost of a bit of bloat per primitive (of which there are typically many).

I guess this should be prioritized higher if this is blocking desktop zooming for 82.

If this is high priority and nobody is working on a fix we can disable the assertion. Myself I'm not going to look into this more since I can't reproduce it and I've already plenty of things to work on.

Flags: needinfo?(nical.bugzilla)

I'm seeing this intermittent occur very frequently with a patch I'm working on too. I can't reproduce locally, but I think I can see what the problem is - will do some try-server debugging today.

The debug assertion check here is trying to validate that we don't
enter an unexpected path for non-tiled images if the image template
has tiling enabled.

However, it was not correctly handling the case where an image key
has been sent but the image was removed. This case is handled by
the resolve_image code below, so it's a valid state to encounter
in this code path.

Assignee: nobody → gwatson
Status: NEW → ASSIGNED
Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4107acc2e89b
Fix incorrect assertion check. r=nical
Blocks: 1665507
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: