Add better error support for CopyFromSurface method [chromium/src : main]
David Bokan (Gerrit)
David Bokan added 5 comments![]( "Open in Gerrit")
const base::expected<viz::CopyOutputBitmapWithMetadata, std::string>&We should add a `using CopyFromSurfaceResult = base:expected...` so that we can use `CopyFromSurfaceResult` instead of this everywhere. It's clearer and if we ever want to update e.g. string -> ErrorCode it would be a simpler change
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap)),Can you file a bug and leave TODOs in places like this...callers need to be updated to handle the returned error explicitly rather than (hopefully) checking for an empty bitmap
if (!result.has_value()) {
// CopyFromSurface failed..
return;
}This changes behavior...if we don't have a result we need to provide an empty one / empty bitmap below.
if (!result.has_value()) {
SupportToolError error = {
SupportToolErrorCode::kDataCollectorError,ditto here and for other cases - aim to leave existing behavior unchanged
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;This is a better pattern to ensure we don't change existing behavior...as noted above, with a TODO to explain that error reporting was added this code was written.
Related details
- Tianyang Xu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tianyang Xu (Gerrit)
Tianyang Xu added 5 comments![]( "Open in Gerrit")
const base::expected<viz::CopyOutputBitmapWithMetadata, std::string>&We should add a `using CopyFromSurfaceResult = base:expected...` so that we can use `CopyFromSurfaceResult` instead of this everywhere. It's clearer and if we ever want to update e.g. string -> ErrorCode it would be a simpler change
Good point. Done
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap)),Can you file a bug and leave TODOs in places like this...callers need to be updated to handle the returned error explicitly rather than (hopefully) checking for an empty bitmap
Create a new ticket b/466199824
if (!result.has_value()) {
// CopyFromSurface failed..
return;
}This changes behavior...if we don't have a result we need to provide an empty one / empty bitmap below.
Done
if (!result.has_value()) {
SupportToolError error = {
SupportToolErrorCode::kDataCollectorError,ditto here and for other cases - aim to leave existing behavior unchanged
Done
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;This is a better pattern to ensure we don't change existing behavior...as noted above, with a TODO to explain that error reporting was added this code was written.
- David Bokan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 5 comments![]( "Open in Gerrit")
Thanks - few small comments but looks good otherwise.
Main question is whether we can be sure now that if we have a result it must be `!drawsNothing`? Unless we're certain it might be better to leave all the existing checks for it
if (thumbnail.drawsNothing()) {
return;
}Do we guarantee now that if we have a bit map it must be `!drawsNothing`?
// if (!result.has_value() || !image_options.has_value()) {
// std::move(callback).Run(SkBitmap());
// return;
// }
// const SkBitmap& screenshot = result->bitmap;Remove
base::expected<CopyOutputBitmapWithMetadata, std::string>I think we should be able to use content::CopyFromSurface result here?
using CopyFromSurfaceResult =(Sorry, I know this will cause a lot of updates but hopefully you can search and replace it mechanically)
I think it's unusual to include qualifiers like const/& in the alias. i.e. this should be:
```
using CopyFromSurfaceResult = base::expected<...>;
```
And call sites should use `const CopyFromSurfaceResult&`.
Related details
- Tianyang Xu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tianyang Xu (Gerrit)
Tianyang Xu added 5 comments![]( "Open in Gerrit")
if (thumbnail.drawsNothing()) {
return;
}Do we guarantee now that if we have a bit map it must be `!drawsNothing`?
Good point, right now it only report error when drawsNothing. But someday it might import more errors. Changed to using value_or() like other places.
// if (!result.has_value() || !image_options.has_value()) {
// std::move(callback).Run(SkBitmap());
// return;
// }
// const SkBitmap& screenshot = result->bitmap;Tianyang XuRemove
Done
base::expected<CopyOutputBitmapWithMetadata, std::string>I think we should be able to use content::CopyFromSurface result here?
I tried to change it to `content::CopyFromSurface`. But with including `"content/public/browser/render_widget_host_view.h"`, I always get the following error:
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
components/viz/common/frame_sinks/copy_output_result.h
Illegal include: "content/public/browser/render_widget_host_view.h"
Because of no rule applying.
```
I tried to add `content/public/browser` in `components/viz/common/frame_sinks/DEPS` and `components/viz/common/BUILD.gn` but just won't work. I wonder if it is because a reverse include, that viz cannot include anything from content.
Or maybe we can put CopyFromSurface in `viz` with a different name?
(Sorry, I know this will cause a lot of updates but hopefully you can search and replace it mechanically)
I think it's unusual to include qualifiers like const/& in the alias. i.e. this should be:
```
using CopyFromSurfaceResult = base::expected<...>;
```And call sites should use `const CopyFromSurfaceResult&`.
Np, it's not too hard to change with Gemini CLI
void(const base::expected<viz::CopyOutputBitmapWithMetadata,
std::string>&)> callback,@bo...@chromium.org These two files are facing similar issue:
They cannot import `render_widget_host_view.h` due to a loop deps.
Related details
- David Bokan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
3 comments
base::expected<CopyOutputBitmapWithMetadata, std::string>Tianyang XuI think we should be able to use content::CopyFromSurface result here?
I tried to change it to `content::CopyFromSurface`. But with including `"content/public/browser/render_widget_host_view.h"`, I always get the following error:
```
** Presubmit ERRORS: 1 **
You added one or more #includes that violate checkdeps rules.
components/viz/common/frame_sinks/copy_output_result.h
Illegal include: "content/public/browser/render_widget_host_view.h"
Because of no rule applying.
```
I tried to add `content/public/browser` in `components/viz/common/frame_sinks/DEPS` and `components/viz/common/BUILD.gn` but just won't work. I wonder if it is because a reverse include, that viz cannot include anything from content.Or maybe we can put CopyFromSurface in `viz` with a different name?
I see, yes, it looks like this is because content depends on components/viz in at least a few places...not least of which is this use case in which RenderWidgetHostView issues a viz::CopyOutputRequest.
This seems like inverted layering to me but is long standing and probably legacy. Viz might be a "special" component. Ok this is fine as is.
if (bitmap.drawsNothing()) {Note: I expect this is likely going to be often what we receive so not super helpful - but lets land this CL as is since it's a lot of mechanical work to update call sites. In a followup CL we can instrument the viz code that fills out CopyOutputResult to also include an error reason which we can plumb through here.
void(const base::expected<viz::CopyOutputBitmapWithMetadata,
std::string>&)> callback,@bo...@chromium.org These two files are facing similar issue:
They cannot import `render_widget_host_view.h` due to a loop deps.
- Tianyang Xu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
- Andrey Kosyakov
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- Tianyang Xu
- thefrog
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
1 comment
Related details
- Andrey Kosyakov
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Tianyang Xu
- thefrog
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +0 |
1 comment
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I believe this is unsafe in the error case because `bitmap` is set to a reference to the temporary default value not the value that is stored by `result`.
Related details
- Andrey Kosyakov
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Tianyang Xu
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I believe this is unsafe in the error case because `bitmap` is set to a reference to the temporary default value not the value that is stored by `result`.
Yikes! Thanks for catching this! Yeah, that's a problem...
Tianyang, the resolution will depend on call site...ideally if the callsite already checks for `drawsNothing()` like here, we can replace that with `result.has_value()`
Otherwise you could construct an empty SkBitmap but you'll have to be careful about lifetime; if that gets passed into async callbacks without a copy you'd need to modify the callbacks to perhaps take CopyFromSurfaceResult instead.
Related details
- Andrey Kosyakov
Tianyang Xu (Gerrit)
Tianyang Xu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tianyang Xu
| Commit-Queue | +1 |
1 comment
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;David BokanI believe this is unsafe in the error case because `bitmap` is set to a reference to the temporary default value not the value that is stored by `result`.
Yikes! Thanks for catching this! Yeah, that's a problem...
Tianyang, the resolution will depend on call site...ideally if the callsite already checks for `drawsNothing()` like here, we can replace that with `result.has_value()`
Otherwise you could construct an empty SkBitmap but you'll have to be careful about lifetime; if that gets passed into async callbacks without a copy you'd need to modify the callbacks to perhaps take CopyFromSurfaceResult instead.
Actually I have checked this before using this `value_or()`. This is what I investigated:
When result has no value, result.value_or(viz::CopyOutputBitmapWithMetadata()) produces a temporary viz::CopyOutputBitmapWithMetadata. The const SkBitmap& bitmap is bound to the bitmap member inside this temporary viz::CopyOutputBitmapWithMetadata. Because bitmap is a const reference, and it is bound to a member of a temporary object, according to the reference lifetime extension rules in the C++ Standard (often called 'reference lifetime extension'), the lifetime of this temporary viz::CopyOutputBitmapWithMetadata object will be extended until the bitmap const reference goes out of scope.
Related details
- Andrey Kosyakov
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- thefrog
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;David BokanI believe this is unsafe in the error case because `bitmap` is set to a reference to the temporary default value not the value that is stored by `result`.
Tianyang XuYikes! Thanks for catching this! Yeah, that's a problem...
Tianyang, the resolution will depend on call site...ideally if the callsite already checks for `drawsNothing()` like here, we can replace that with `result.has_value()`
Otherwise you could construct an empty SkBitmap but you'll have to be careful about lifetime; if that gets passed into async callbacks without a copy you'd need to modify the callbacks to perhaps take CopyFromSurfaceResult instead.
Actually I have checked this before using this `value_or()`. This is what I investigated:
When result has no value, result.value_or(viz::CopyOutputBitmapWithMetadata()) produces a temporary viz::CopyOutputBitmapWithMetadata. The const SkBitmap& bitmap is bound to the bitmap member inside this temporary viz::CopyOutputBitmapWithMetadata. Because bitmap is a const reference, and it is bound to a member of a temporary object, according to the reference lifetime extension rules in the C++ Standard (often called 'reference lifetime extension'), the lifetime of this temporary viz::CopyOutputBitmapWithMetadata object will be extended until the bitmap const reference goes out of scope.
OK I did a dipper research, it turns out reference lifetime extension only apply to a top class/struct itself, but not to a member inside a temp object. So using a `const CopyOutputBitmapWithMetadata&` is safe with `value_or()`, but a `const SkBitmap&` is not. Let me see what I can do for this scenario.
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
if (bitmap.drawsNothing()) {Should this and L321 instead be changed to result.has_value() now? This way we would be able to ditch L317 altogether and just use result->bitmap in L326.
Related details
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- Tianyang Xu
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Duncan Mercer (Gerrit)
Duncan Mercer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Duncan Mercer
| Code-Review | +1 |
1 comment
Related details
- David Bokan
Khushal Sagar (Gerrit)
Khushal Sagar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Khushal Sagar
| Code-Review | +1 |
1 comment
// Returns a `base::expected` containing `CopyOutputBitmapWithMetadata`
// on success, or an `std::string` error on failure.
// `CopyOutputBitmapWithMetadata` contains a `SkBitmap` which can be used out
// the scope of the `ScopedSkBitmap` and makes a copy of the content in
// `CopyOutputResult` if needed.
base::expected<CopyOutputBitmapWithMetadata, std::string>
GetOutScopedBitmapAndMetadata() const;nit: Do you need the viz API to change for this? The only possible failure is "no bitmap available" which can be done at the call-site. This is helpful if a follow up change will add more explicit errors from the GPU process back to the browser. Otherwise I'd prefer leaving this as-is for now.
Related details
- David Bokan
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
// Returns a `base::expected` containing `CopyOutputBitmapWithMetadata`
// on success, or an `std::string` error on failure.
// `CopyOutputBitmapWithMetadata` contains a `SkBitmap` which can be used out
// the scope of the `ScopedSkBitmap` and makes a copy of the content in
// `CopyOutputResult` if needed.
base::expected<CopyOutputBitmapWithMetadata, std::string>
GetOutScopedBitmapAndMetadata() const;nit: Do you need the viz API to change for this? The only possible failure is "no bitmap available" which can be done at the call-site. This is helpful if a follow up change will add more explicit errors from the GPU process back to the browser. Otherwise I'd prefer leaving this as-is for now.
I made a similar comment in https://chromium-review.googlesource.com/c/chromium/src/+/7229082/comment/eab99fca_17cf31dd/ but expect that we'll follow up here since there's not many early-outs before this...I expect the real failures will come from inside Viz.
I like this split because this CL does the mostly mechanical changes to client call sites and the followup will actually instrument the viz paths...but given this adds no benefit without the followup perhaps we can wait until that's ready and land them together?
Related details
David Bokan (Gerrit)
David Bokan added 9 comments![]( "Open in Gerrit")
if (bitmap.drawsNothing() || drop_after_readback_) {nit: check result.has_value() here.
if (bitmap.drawsNothing() && remaining_retries > 0) {check result.has_value() instead.
return result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I think this won't work in the failure case? Because this is a temporary but OnSnapshotTakenForOnDeviceModel takes the argument by ref. I think we can modify OnSnapshotTakenForOnDeviceModel to take a base::expected or SkBitmap* with nullptr for error case.
if (bitmap.drawsNothing() || layer_size.width() * layer_size.height() <= 0 ||Check result.has_value() instead and then you can use `result->bitmap`
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I suspect this pattern still leads to a dangling ref because, IIUC, the temporary object lifetime extension applies only if you're holding a ref to the temporary object itself. i.e. here the `CopyOutputBitmapWithMetadata` will be destroyed (along with its inner `bitmap`) at the end of the line, despite you holding a ref to the inner `bitmap`. I think you'd either have to hold a ref to `CopyOutputBitmapWithMetadata` or do something like:
```
const SkBitmap& bitmap = result.has_value() ? result->bitmap : SkBitmap()
```
Please update all uses of this pattern
++screenshot_attempt_id_, bitmap, std::move(callback)));Hmm, this seems suspect before your change...this is a reply callback for a mojo call (so async) but takes the bitmap by ref...
I suspect GetPdfCurrentPage (and inner uses) should be taking the SkBitmap by value (which shares buffers on construction using ref-counting)...perhaps @mer...@google.com can comment on the way forward here...
// error case.
return result
.value_or(viz::CopyOutputBitmapWithMetadata())
.bitmap;
})Here also, we'll need to pass a nullptr or something else in the failure case since the on_bitmap_fetched callback takes by-ref.
// TODO(crbug.com/466199824): Update callsite to handle error case.
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;StoreThumbnail just early outs if we have no value...I think we can do that here rather than passing in the empty ref (after the UMA_HISTOGRAM to preserve behavior), i.e.:
```
UMA_HISTOGRAM...
if (!result.has_value()) {
return;
}
StoreThumbnail(..., result->bitmap, ...)
```
if (bitmap.drawsNothing()) {Should this and L321 instead be changed to result.has_value() now? This way we would be able to ditch L317 altogether and just use result->bitmap in L326.
+1 we should do this wherever nearby code is checking `drawsNothing`
thefrog (Gerrit)
thefrog added 1 comment![]( "Open in Gerrit")
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I think this has the same issue discussed in other comments right?
Related details
- Kyle Horimoto
- Reilly Grant
- Tianyang Xu
Tianyang Xu (Gerrit)
Tianyang Xu added 8 comments![]( "Open in Gerrit")
if (bitmap.drawsNothing() || drop_after_readback_) {Tianyang Xunit: check result.has_value() here.
Done
if (bitmap.drawsNothing() && remaining_retries > 0) {Tianyang Xucheck result.has_value() instead.
Done
return result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I think this won't work in the failure case? Because this is a temporary but OnSnapshotTakenForOnDeviceModel takes the argument by ref. I think we can modify OnSnapshotTakenForOnDeviceModel to take a base::expected or SkBitmap* with nullptr for error case.
Changed to base::expected<SkBitmap, string> in OnSnapshotTakenForOnDeviceModel
if (bitmap.drawsNothing() || layer_size.width() * layer_size.height() <= 0 ||Check result.has_value() instead and then you can use `result->bitmap`
Done
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I suspect this pattern still leads to a dangling ref because, IIUC, the temporary object lifetime extension applies only if you're holding a ref to the temporary object itself. i.e. here the `CopyOutputBitmapWithMetadata` will be destroyed (along with its inner `bitmap`) at the end of the line, despite you holding a ref to the inner `bitmap`. I think you'd either have to hold a ref to `CopyOutputBitmapWithMetadata` or do something like:
```
const SkBitmap& bitmap = result.has_value() ? result->bitmap : SkBitmap()
```Please update all uses of this pattern
Yep, this is what I investigated, that the const ref should not be used on the bitmap which is part of the temp CopyOutputBitmapWithMetadata().
// TODO(crbug.com/466199824): Update callsite to handle error case.
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;StoreThumbnail just early outs if we have no value...I think we can do that here rather than passing in the empty ref (after the UMA_HISTOGRAM to preserve behavior), i.e.:
```
UMA_HISTOGRAM...
if (!result.has_value()) {
return;
}
StoreThumbnail(..., result->bitmap, ...)
```
Done
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;I think this has the same issue discussed in other comments right?
Yes, changed to
const SkBitmap& bitmap = result.has_value() ? result->bitmap : SkBitmap();
to avoid dangling reference.
David BokanShould this and L321 instead be changed to result.has_value() now? This way we would be able to ditch L317 altogether and just use result->bitmap in L326.
+1 we should do this wherever nearby code is checking `drawsNothing`
- Andrey Kosyakov
- Avi Drissman
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
2 comments
const SkBitmap& bitmap = result.has_value() ? result->bitmap : SkBitmap();you don't need this here anymore, just inline result->bitmap() in L326 since at that point we already know it has value.
Related details
- Avi Drissman
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- Tianyang Xu
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
1 comment
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Tianyang Xu
- thefrog
thefrog (Gerrit)
thefrog voted and added 1 comment![]( "Open in Gerrit")
Votes added by thefrog
| Code-Review | +1 |
1 comment
safe_browsing LGTM
Related details
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Tianyang Xu
Tianyang Xu (Gerrit)
Tianyang Xu added 1 comment![]( "Open in Gerrit")
const SkBitmap& bitmap = result.has_value() ? result->bitmap : SkBitmap();you don't need this here anymore, just inline result->bitmap() in L326 since at that point we already know it has value.
- Andrey Kosyakov
- David Bokan
- Duncan Mercer
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Duncan Mercer (Gerrit)
Duncan Mercer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Duncan Mercer
| Code-Review | +1 |
1 comment
++screenshot_attempt_id_, bitmap, std::move(callback)));Hmm, this seems suspect before your change...this is a reply callback for a mojo call (so async) but takes the bitmap by ref...
I suspect GetPdfCurrentPage (and inner uses) should be taking the SkBitmap by value (which shares buffers on construction using ref-counting)...perhaps @mer...@google.com can comment on the way forward here...
Hmmm. This is a well tested path thats never caused issues. However, if you think moving to passing by value is safer, SGTM.
Related details
- Andrey Kosyakov
- David Bokan
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- Tianyang Xu
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
thefrog (Gerrit)
thefrog voted and added 1 comment![]( "Open in Gerrit")
Votes added by thefrog
| Code-Review | +1 |
1 comment
safe_browsing/ is unchanged from my last +1
Related details
- Andrey Kosyakov
- David Bokan
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
- Tianyang Xu
Tianyang Xu (Gerrit)
Tianyang Xu added 3 comments![]( "Open in Gerrit")
++screenshot_attempt_id_, bitmap, std::move(callback)));Duncan MercerHmm, this seems suspect before your change...this is a reply callback for a mojo call (so async) but takes the bitmap by ref...
I suspect GetPdfCurrentPage (and inner uses) should be taking the SkBitmap by value (which shares buffers on construction using ref-counting)...perhaps @mer...@google.com can comment on the way forward here...
Hmmm. This is a well tested path thats never caused issues. However, if you think moving to passing by value is safer, SGTM.
Done
// error case.
return result
.value_or(viz::CopyOutputBitmapWithMetadata())
.bitmap;
})Here also, we'll need to pass a nullptr or something else in the failure case since the on_bitmap_fetched callback takes by-ref.
Changed to a value copy
const SkBitmap& bitmap =
result.value_or(viz::CopyOutputBitmapWithMetadata()).bitmap;David BokanI believe this is unsafe in the error case because `bitmap` is set to a reference to the temporary default value not the value that is stored by `result`.
Tianyang XuYikes! Thanks for catching this! Yeah, that's a problem...
Tianyang, the resolution will depend on call site...ideally if the callsite already checks for `drawsNothing()` like here, we can replace that with `result.has_value()`
Otherwise you could construct an empty SkBitmap but you'll have to be careful about lifetime; if that gets passed into async callbacks without a copy you'd need to modify the callbacks to perhaps take CopyFromSurfaceResult instead.
Tianyang XuActually I have checked this before using this `value_or()`. This is what I investigated:
When result has no value, result.value_or(viz::CopyOutputBitmapWithMetadata()) produces a temporary viz::CopyOutputBitmapWithMetadata. The const SkBitmap& bitmap is bound to the bitmap member inside this temporary viz::CopyOutputBitmapWithMetadata. Because bitmap is a const reference, and it is bound to a member of a temporary object, according to the reference lifetime extension rules in the C++ Standard (often called 'reference lifetime extension'), the lifetime of this temporary viz::CopyOutputBitmapWithMetadata object will be extended until the bitmap const reference goes out of scope.
OK I did a dipper research, it turns out reference lifetime extension only apply to a top class/struct itself, but not to a member inside a temp object. So using a `const CopyOutputBitmapWithMetadata&` is safe with `value_or()`, but a `const SkBitmap&` is not. Let me see what I can do for this scenario.
Done
Related details
- Andrey Kosyakov
- David Bokan
- Khushal Sagar
- Kyle Horimoto
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
1 comment
Related details
- Andrey Kosyakov
- David Bokan
- Khushal Sagar
- Kyle Horimoto
- Nihar Majmudar
- Tianyang Xu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Khushal Sagar
| Code-Review | +1 |
2 comments
viz LGTM.
// Returns a `base::expected` containing `CopyOutputBitmapWithMetadata`
// on success, or an `std::string` error on failure.
// `CopyOutputBitmapWithMetadata` contains a `SkBitmap` which can be used out
// the scope of the `ScopedSkBitmap` and makes a copy of the content in
// `CopyOutputResult` if needed.
base::expected<CopyOutputBitmapWithMetadata, std::string>
GetOutScopedBitmapAndMetadata() const;David Bokannit: Do you need the viz API to change for this? The only possible failure is "no bitmap available" which can be done at the call-site. This is helpful if a follow up change will add more explicit errors from the GPU process back to the browser. Otherwise I'd prefer leaving this as-is for now.
I made a similar comment in https://chromium-review.googlesource.com/c/chromium/src/+/7229082/comment/eab99fca_17cf31dd/ but expect that we'll follow up here since there's not many early-outs before this...I expect the real failures will come from inside Viz.
I like this split because this CL does the mostly mechanical changes to client call sites and the followup will actually instrument the viz paths...but given this adds no benefit without the followup perhaps we can wait until that's ready and land them together?
If the plan is to get more specific errors from the Viz code then this LGTM. I'm ok landing this now.
Related details
- Andrey Kosyakov
- David Bokan
- Kyle Horimoto
- Nihar Majmudar
- Tianyang Xu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement