Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3c53ec51 authored by Chris Craik's avatar Chris Craik
Browse files

Prevent EndLayerOps when Begin was rejected 

bug:30537130

BeginLayerOps were being rejected in a way that allowed the associated
EndLayerOps to still be recorded. This was a violation of DisplayList
content expectations, and caused crashes in FrameBuilder when trying to
play these DisplayLists back.

Change-Id: I531b840aa5c4ffb1ee458da3f4b366978eaeafbe
parent a6b1a948

libs/hwui/RecordingCanvas.cpp

+37 −34
Original line number Diff line number Diff line
@@ -149,26 +149,26 @@ int RecordingCanvas::saveLayer(float left, float top, float right, float bottom, 


    // Map visible bounds back to layer space, and intersect with parameter bounds

    Rect layerBounds = visibleBounds;

    if (CC_LIKELY(!layerBounds.isEmpty())) {

        // if non-empty, can safely map by the inverse transform

        Matrix4 inverse;

        inverse.loadInverse(*previous.transform);

        inverse.mapRect(layerBounds);

        layerBounds.doIntersect(unmappedBounds);

    }



    int saveValue = mState.save((int) flags);

    Snapshot& snapshot = *mState.writableSnapshot();



    // layerBounds is in original bounds space, but clipped by current recording clip

    if (layerBounds.isEmpty() || unmappedBounds.isEmpty()) {

        // Don't bother recording layer, since it's been rejected

        if (CC_LIKELY(clippedLayer)) {

            snapshot.resetClip(0, 0, 0, 0);

        }

        return saveValue;

    }



    if (!layerBounds.isEmpty() && !unmappedBounds.isEmpty()) {

        if (CC_LIKELY(clippedLayer)) {

        auto previousClip = getRecordedClip(); // note: done before new snapshot's clip has changed



            auto previousClip = getRecordedClip(); // capture before new snapshot clip has changed

            if (addOp(alloc().create_trivial<BeginLayerOp>(

                    unmappedBounds,

                    *previous.transform, // transform to *draw* with

                    previousClip, // clip to *draw* with

                    refPaint(paint))) >= 0) {

                snapshot.flags |= Snapshot::kFlagIsLayer | Snapshot::kFlagIsFboLayer;

                snapshot.initializeViewport(unmappedBounds.getWidth(), unmappedBounds.getHeight());

                snapshot.transform->loadTranslate(-unmappedBounds.left, -unmappedBounds.top, 0.0f);
@@ -177,22 +177,25 @@ int RecordingCanvas::saveLayer(float left, float top, float right, float bottom, 
                clip.translate(-unmappedBounds.left, -unmappedBounds.top);

                snapshot.resetClip(clip.left, clip.top, clip.right, clip.bottom);

                snapshot.roundRectClipState = nullptr;



        addOp(alloc().create_trivial<BeginLayerOp>(

                unmappedBounds,

                *previous.transform, // transform to *draw* with

                previousClip, // clip to *draw* with

                refPaint(paint)));

                return saveValue;

            }

        } else {

        snapshot.flags |= Snapshot::kFlagIsLayer;



        addOp(alloc().create_trivial<BeginUnclippedLayerOp>(

            if (addOp(alloc().create_trivial<BeginUnclippedLayerOp>(

                    unmappedBounds,

                    *mState.currentSnapshot()->transform,

                    getRecordedClip(),

                refPaint(paint)));

                    refPaint(paint))) >= 0) {

                snapshot.flags |= Snapshot::kFlagIsLayer;

                return saveValue;

            }

        }

    }



    // Layer not needed, so skip recording it...

    if (CC_LIKELY(clippedLayer)) {

        // ... and set empty clip to reject inner content, if possible

        snapshot.resetClip(0, 0, 0, 0);

    }

    return saveValue;

}
@@ -619,7 +622,7 @@ void RecordingCanvas::callDrawGLFunction(Functor* functor, 
            functor));

}



size_t RecordingCanvas::addOp(RecordedOp* op) {

int RecordingCanvas::addOp(RecordedOp* op) {

    // skip op with empty clip

    if (op->localClip && op->localClip->rect.isEmpty()) {

        // NOTE: this rejection happens after op construction/content ref-ing, so content ref'd

libs/hwui/RecordingCanvas.h

+1 −1
Original line number Diff line number Diff line
@@ -208,7 +208,7 @@ private: 
    void drawSimpleRects(const float* rects, int vertexCount, const SkPaint* paint);





    size_t addOp(RecordedOp* op);

    int addOp(RecordedOp* op);

// ----------------------------------------------------------------------------

// lazy object copy

// ----------------------------------------------------------------------------

libs/hwui/tests/unit/RecordingCanvasTests.cpp

+15 −0
Original line number Diff line number Diff line
@@ -545,6 +545,21 @@ TEST(RecordingCanvas, saveLayer_rotateClipped) { 
    EXPECT_EQ(3, count);

}



TEST(RecordingCanvas, saveLayer_rejectBegin) {

    auto dl = TestUtils::createDisplayList<RecordingCanvas>(200, 200, [](RecordingCanvas& canvas) {

        canvas.save(SaveFlags::MatrixClip);

        canvas.translate(0, -20); // avoid identity case

        // empty clip rect should force layer + contents to be rejected

        canvas.clipRect(0, -20, 200, -20, SkRegion::kIntersect_Op);

        canvas.saveLayerAlpha(0, 0, 200, 200, 128, SaveFlags::ClipToLayer);

        canvas.drawRect(0, 0, 200, 200, SkPaint());

        canvas.restore();

        canvas.restore();

    });



    ASSERT_EQ(0u, dl->getOps().size()) << "Begin/Rect/End should all be rejected.";

}



TEST(RecordingCanvas, drawRenderNode_rejection) {

    auto child = TestUtils::createNode(50, 50, 150, 150,

            [](RenderProperties& props, RecordingCanvas& canvas) {