980233 - Use RAII for DataSourceSurface::Map to make it safer by making it impossible to forget to Unmap

Status: NEW

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

DataSourceSurface::Map is a bit clunky to use, partly due to its boolean return value when what users really want to know is whether map.mData is null or not. That either requires creating a separate variable and checking both, as so:

  DataSourceSurface::MappedSurface map;
  bool ok = dataSurface->Map(DataSourceSurface::MapType::READ, &map);
  if (!ok || !map.mData) {
    // handle failure
  }

or else putting the Map() call into the |if| statement's parenthetical which is against style rules for getter calls in most parts of the codebase:

  DataSourceSurface::MappedSurface map;
  if (!dataSurface->Map(DataSourceSurface::MapType::READ, &map) ||
      !map.mData) {
    // handle failure
  }

Is there a reason that DataSourceSurface::Map has to map the stride? Is that going to be different for different MapType values? If not it would seem to be nicer to just return a pointer to the data.

  uint8_t* data = dataSurface->MapData(DataSourceSurface::MapType::READ);
  if (!data) {
    // handle failure
  }

and access Stride() directly from the dataSurface as needed.

Alternatively, can we just get rid of the boolean return value and say that success or failure is indicated by whether map.mData is null or not?

Comment 1

[Jonathan Watt [:jwatt]]

I'm also thinking the Unmap mechanism's could make it harder for users to screw up. Ideally we'd unmap when the MappedSurface does out of scope, and null out the MappedSurface's mData member if the user calls Unmap (to prevent hard to track down bugs if mData is used after unmapping).

Comment 2

[Jonathan Watt [:jwatt]]

Bug 979853 comment 6 notes that we're already screwing up the Unmap() calls in the codebase, and calling DataSourceSurfaceD2D::Unmap() even seems guaranteed to crash debug builds. Unfortunately we never did add a RAII class as suggested in bug 960254 comment 10. I think we should do that.

Comment 3

[Jonathan Watt [:jwatt]]

Let's just use this bug for RAII discussions.

Regarding my comment 0 suggestion of returning the data pointer directly, Bas tells me that the stride is explicitly part of the map for D3D, and it can in principle return a different stride each time you map. Therefore the stride and data both have to be returned from the Map() call. The convention that Bas prefers is the D2D convention of returning a bool, promising that if the bool is true than mData is populated and if it's false that mData is undefined. Therefore callers are encouraged to check the bool and if it's true assume mData is set. Personally I'd prefer to drop the redundant bool and just check mData (promising it's nulled out on failure), but whatever, that's a small annoyance and not worth arguing about.

Comment 4

[Jonathan Watt [:jwatt]]

I talked with Bas about having a RAII class on IRC, which I think I should summarize here. He seemed to object to making a RAII class part of the API for several reasons including:

 1) "I dislike it when RAII is used to paper over incompetence, which it
    often is :)"

 2) it's not free

 3) "When it's a small block with no exit paths, explicit is better"

 4) "RAII makes people lazy, it makes people stop thinking about when
    they should do something optimally, and rather just go 'meh, I'll
    get rid of it whenever this goes out of scope, whenever that is'"

Bas will accept a RAII class in a utils file, but I think it should be the default otherwise we're inevitably going to just end up with bugs creeping back in (after we've fixed the existing Unmap failures). I think this should trump any concerns about miniscule perf impact of a ctor/dtor.

Comment 5

[Jonathan Watt [:jwatt]]

Besides all the locations I fixed in bug 980415 and bug 980436, bug 979394 just introduced yet more code that fails to Unmap().

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Watt [:jwatt] from comment #4)
>  1) "I dislike it when RAII is used to paper over incompetence, which it
>     often is :)"
> 
>  2) it's not free
> 
>  3) "When it's a small block with no exit paths, explicit is better"
> 
>  4) "RAII makes people lazy, it makes people stop thinking about when
>     they should do something optimally, and rather just go 'meh, I'll
>     get rid of it whenever this goes out of scope, whenever that is'"

As far as I can tell these are generic arguments against any use of RAII. I think there's a pretty clear consensus within Gecko that RAII is preferred over explicit acquire/release. In general we'd rather have code patterns that minimize the chances of error even if this can be described as "papering over incompetence".

So I agree that we should make RAII mandatory here.

If Bas still doesn't agree I'm happy to raise the issue on dev-platform and see if there is any support for his position.

Comment 7

[Andreas Gal :gal]

I agree as well that this should be mandatory. There is no point in bringing this to dev-platform. Everyone there will agree as well that explicit management of exit paths is never a sane idea. I will catch up tomorrow with Bas and make sure he agrees. In the meantime lets go ahead and design a RAII API for this.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Jonathan Watt [:jwatt] from comment #4)
> >  1) "I dislike it when RAII is used to paper over incompetence, which it
> >     often is :)"
> > 
> >  2) it's not free
> > 
> >  3) "When it's a small block with no exit paths, explicit is better"
> > 
> >  4) "RAII makes people lazy, it makes people stop thinking about when
> >     they should do something optimally, and rather just go 'meh, I'll
> >     get rid of it whenever this goes out of scope, whenever that is'"
> 
> As far as I can tell these are generic arguments against any use of RAII. I
> think there's a pretty clear consensus within Gecko that RAII is preferred
> over explicit acquire/release. In general we'd rather have code patterns
> that minimize the chances of error even if this can be described as
> "papering over incompetence".
> 
> So I agree that we should make RAII mandatory here.
> 
> If Bas still doesn't agree I'm happy to raise the issue on dev-platform and
> see if there is any support for his position.

I think it's not a bad idea to have an RAII helper when there is multiple exit paths. I might even agree it's a good idea then. However there's some cases where you want to explicitly manage the lifetime, and managing it form an RAII helper class becomes annoying then, so having that as the only option is a good idea. I also think in the situation of:

Map();
Write();
Unmap();

With no intermediate exit paths explicit maps and unmaps are clearly preferable as they expose directly the programmer's intent rather than hiding the time of Unmap. This increases the deterministicness of the code (and even the performance, if the Map holds an internal lock on something). Of course we could enforce everyone who uses the RAII code to call unmap as soon as they can or scope explicitly, but this will often just be ignored out of laziness.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Jonathan Watt [:jwatt] from comment #4)
> 
> So I agree that we should make RAII mandatory here.
> 
> If Bas still doesn't agree I'm happy to raise the issue on dev-platform and
> see if there is any support for his position.

Fwiw, I do not agree. However I am sure dev-platform will agree with you (because we tend to be inherently lazy), so whether you bring it up or not I leave up to you, I don't think there's much value in bringing it up.

I don't have the time or the energy to fight this battle so I won't comment on it. If you guys want to change this then I'm not going to bother standing in your way :).

Comment 10

[Bas Schouten (:bas.schouten)]

Just as a concrete example of a place where I don't think this would make things prettier. When I last looked at Moz2D-ifying our image decoding code we (from outside code) call a sort of 'lock for writing' function which gets our image ready for decoding into, and then we call a sort of 'unlock' function later. This outside code has no access directly to the Moz2D structures, so in the case of an RAII structure being mandatory, you'd somehow have to manage the lifetime on the object. There's obviously several solutions that can be thought of, but in my opinion none of them offer the elegance of the 'Lock' function calling Map, and the 'Unlock' function calling Unmap explicitly.

Comment 11

[Andreas Gal :gal]

Bas, why is 

{
  RAII raii;

}

Not giving someone the same degree of control?

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Andreas Gal :gal from comment #11)
> Bas, why is 
> 
> {
>   RAII raii;
> 
> }
> 
> Not giving someone the same degree of control?

Twofold, first of all, I'm worried most people, when forced to use the RAII, won't bother to do it :). Second it doesn't easily enable the scenario above when you need preservation of the map outside of the scope. Unless you have special assignment operators and such. In which case you'd almost consider a refcounted 'Map' class.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> Just as a concrete example of a place where I don't think this would make
> things prettier. When I last looked at Moz2D-ifying our image decoding code
> we (from outside code) call a sort of 'lock for writing' function which gets
> our image ready for decoding into, and then we call a sort of 'unlock'
> function later. This outside code has no access directly to the Moz2D
> structures, so in the case of an RAII structure being mandatory, you'd
> somehow have to manage the lifetime on the object. There's obviously several
> solutions that can be thought of, but in my opinion none of them offer the
> elegance of the 'Lock' function calling Map, and the 'Unlock' function
> calling Unmap explicitly.

In that case you should perhaps wrap up the "lock for writing" and "unlock" functions in their own RAII object. Hard to say without digging into the specific code.

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> I don't have the time or the energy to fight this battle so I won't comment
> on it. If you guys want to change this then I'm not going to bother standing
> in your way :).

Thanks. jwatt, let's do it :-).