Convert the Windows widget consumers of imgIContainer::GetFrame to act on a Moz2D SourceSurface instead of a Thebes gfxASurface

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Depends on 1 bug, Blocks 1 bug)

29 Branch
mozilla30
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
As part of converting imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a Thebes gfxASurface, we should convert the Windows widget consumers of imgIContainer::GetFrame to act on a Moz2D SourceSurface instead of a Thebes gfxASurface.
(Assignee)

Comment 1

5 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8386065 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

5 years ago
Posted patch patchSplinter Review
Attachment #8386065 - Attachment is obsolete: true
Attachment #8386065 - Flags: review?(matt.woodrow)
Attachment #8386067 - Flags: review?(matt.woodrow)
Comment on attachment 8386067 [details] [diff] [review]
patch

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

::: widget/windows/nsImageClipboard.cpp
@@ +129,5 @@
> +      thebesSurface->GetAsReadableARGB32ImageSurface();
> +    NS_ENSURE_TRUE(thebesImageSurface, NS_ERROR_FAILURE);
> +
> +    RefPtr<DataSourceSurface> dataSurface =
> +      thebesImageSurface->CopyToB8G8R8A8DataSourceSurface();

Should use Factory::CreateWrappingDataSurface here instead of copying.

::: widget/windows/nsWindowGfx.cpp
@@ +688,5 @@
> +  } else {
> +    dataSurface = surface->GetDataSurface();
> +    dataSurface->Map(DataSourceSurface::MapType::READ, &map);
> +  }
> +  NS_ENSURE_TRUE(dataSurface && map.mData, NS_ERROR_FAILURE);

map.mData is probably uninitialized if Map() failed. We should check that return value instead.

@@ +693,5 @@
> +  MOZ_ASSERT(dataSurface->GetFormat() == SurfaceFormat::B8G8R8A8);
> +
> +  uint8_t* data = nullptr;
> +  nsAutoArrayPtr<uint8_t> autoDeleteArray;
> +  if (map.mStride == 4 * sizeof(uint8_t) * iconSize.width) {

gfx/2d/Tools.h has BytesPerPixel(SurfaceFormat) for this.
Attachment #8386067 - Flags: review?(matt.woodrow) → review+
(In reply to Jonathan Watt [:jwatt] from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/22e34e33e9ee

hi sorry had to backout this change for win7/8 debug crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=35715170&tree=Mozilla-Inbound
(Assignee)

Comment 6

5 years ago
This is because I added a "missing" Unmap() call and we now hit the MOZ_ASSERT(mIsMapped) in DataSourceSurfaceD2D::Unmap(). It seems DataSourceSurfaceD2D::Map doesn't actually set mIsMapped, so I don't see how this can work anywhere else in the codebase unless all those calls are either (a) not on a DataSourceSurfaceD2D DataSourceSurface, or (b) the Unmap() calls are missing. I've noticed that the latter is the case in some places (Unmap calls are missing).

In other words I seem to have stumbled upon an existing DataSourceSurfaceD2D::Map bug, and some existing Unmap-missing bugs.
https://hg.mozilla.org/mozilla-central/rev/4efacf2f947b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

5 years ago
Depends on: 981020
(Assignee)

Updated

5 years ago
Depends on: 982697
(Assignee)

Updated

5 years ago
Depends on: 985477
Depends on: CVE-2014-1528
You need to log in before you can comment on or make changes to this bug.