528731 - Potential licensing problems in nsDataObj.cpp from Bug 374593

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Parts of nsDataObj.cpp are copied directly from Raymond Chen's MSDN article on the Drag and Drop helper classes (see URL).

In particular, it appears that almost all of this diff http://hg.mozilla.org/mozilla-central/diff/f4244e74c70d/widget/src/windows/nsDataObj.cpp is copied from that article.  This was added by bug 374593.

MSDN says sample code is released under the Microsoft Limited Public License http://msdn.microsoft.com/en-us/cc300389.aspx#D which is GPL-incompatible.

It seems to me that we can't use this code.  Ccing shaver per robarnold on IRC.

Comment 1

[Gervase Markham [:gerv]]

I agree that the MSLPL isn't compatible with (any) open-source licence (it restricts you to only using the code on Windows OSes) and it does seem that all MSDN code is covered by that licence.

I also think that the code is too similar for this to be coincidence - there has been copying and pasting. Check, for example, the function parameter variable names.

CCing reed, who landed it.

Gerv

Comment 2

[Benjamin Smedberg]

IANAL, but I don't think there's a creative work which can be copyrighted here: this is boilerplate-type code, with standardized variable names and prefixes, and the correct code pattern would look the same no matter who wrote it.

Comment 3

[Reed Loden [:reed]]

(In reply to comment #1)
> CCing reed, who landed it.

Just because I landed it doesn't mean I actually have any knowledge of what it is or know everything about it. I've landed a lot of code for a lot of people over the years as somebody who helps out with checkin-needed runs. You should always look into who actually wrote it rather than who committed it.

CC'ing jmathies, who actually wrote it.

Comment 4

[Gervase Markham [:gerv]]

bsmedberg: I defer to your superior knowledge of Windows hackery and conventions, but if this code is so uncreative, why would there need to be an MSDN article explaining how to do this task? IMO the amount of code copied is sufficient that Microsoft have a copyright interest in it. CCing Harvey.

(In reply to comment #3)
> Just because I landed it doesn't mean I actually have any knowledge of what it
> is or know everything about it. 

http://www.mozilla.org/hacking/committer/committers-agreement.pdf :

"4. Committing Code Created by Others

You may check in Code to a Mozilla Foundation repository that was not written by You, provided that:
...
b) You make all reasonable and appropriate efforts to ensure that such Code conforms to the terms of this agreement."

That makes it, at least to some extent, your problem too. (If this means indirectly that checkin-needed needs to start working differently, then that's what it means.)

Gerv

Comment 5

[harvey anderson]

Absent license compatibility, recommend we find an alternative way to include this functionality or confirm that it was original code. I would not rely on minimum amount of copied code rationale.

Comment 6

[Jim Mathies [:jimm]]

Why ms would ding us legally for using example code on msdn to produce solid drag and drop functionality? If we simply want to reformat, that's fine, but the functionality is going to remain the same.

Comment 7

[Reed Loden [:reed]]

(In reply to comment #4)
> (In reply to comment #3)
> > Just because I landed it doesn't mean I actually have any knowledge of what it
> > is or know everything about it. 
> 
> http://www.mozilla.org/hacking/committer/committers-agreement.pdf :
<snip>
> That makes it, at least to some extent, your problem too. (If this means
> indirectly that checkin-needed needs to start working differently, then that's
> what it means.)

Yes, I'm aware of what the policy says, but it's really just not feasible at all for the committer of somebody else's patch to know 100% that such patch does not infringe upon our licenses or any other related licenses that the code may be under (such as in this case), especially when it's unlikely that the committer was one of the actual patch reviewers and has little to no knowledge of what the patch does. There are obvious cases that can be caught, and I personally have caught several over the years and worked to get them fixed, but in general, it just isn't possible.

If you actually want to enforce the clause, I can promise you that very few to no people (even less than the few who do it currently!) will want to handle checkin-needed duty, lest they fall prey to the same implications, as it really just isn't worth the trouble it may cause later down the road. Patches will pile up, and it'll just be a large mess for all. So, yes, something needs to be done if you actually want code from non-committers to land in the tree. Perhaps a thread needs to be started in mozilla.governance to discuss this?

However, I'm more than happy to back out the offending code on trunk and all affected branches, as it's really not worth my time and the legal implications it could cause me to leave it in now that this issue is known. Unless I hear otherwise and see active progress on getting this resolved as quickly as possible, I will do just that later today.

Do note that in bug 374593, comment 40, it does seem at least one of the reviewers was aware that part of this code came from MSDN. Perhaps more education needs to be done to make sure reviewers are aware of possible legal/licensing problems that can result from copying code from elsewhere?

Comment 8

[Mike Shaver (:shaver emeritus)]

What do you mean "ding us legally"?  Just because they wouldn't press a suit doesn't mean that we can't relicense their code unilaterally.  They presumably chose the source license for their code samples for a reason, and could have chosen one that was more liberal.

Developers should not be copying code into our source tree on the basis that they think the author won't sue for copyright violation.

Copyright doesn't protect functionality, it protects expression.  If this is the only possible expression, which I doubt, then we should review that.

Comment 9

[Mike Shaver (:shaver emeritus)]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Just because I landed it doesn't mean I actually have any knowledge of what it
> > > is or know everything about it. 
> > 
> > http://www.mozilla.org/hacking/committer/committers-agreement.pdf :
> <snip>
> > That makes it, at least to some extent, your problem too. (If this means
> > indirectly that checkin-needed needs to start working differently, then that's
> > what it means.)
> 
> Yes, I'm aware of what the policy says, but it's really just not feasible at
> all for the committer of somebody else's patch to know 100%

That's not what the policy says, though: it says reasonable efforts, and I think tbh that "this patch came from a Mozilla employee" is a reasonable effort in this case.  We should all understand what we can and can't do with other-licensed code, and how to confirm that if we're not certain; if we don't, it's my problem to solve, I think.

Comment 10

[Gervase Markham [:gerv]]

Actually, again, Shaver is right. While I may have reservations about checkin-needed from this POV, it was reasonable for reed to note that a Mozilla employee wrote the code.

I will have a think about if, where and how to take this issue forward (in consultation with shaver).

Gerv

Comment 11

[Jim Mathies [:jimm]]

If the concern here is simply that it too closely emulates the *format* of code on msdn (which is understandable, it is a copy of that code)  then I can easily mung this up in a way that makes it look completely different but still does the same thing. The functionality though needs to remain, we really shouldn't be ripping out code that provides needed functionality simply because there's an example on msdn that shows how it's to be done.

Another question I have is, where is the line to be drawn? If I read "int i = 0;" on msdn, and use it in my code, am I violating copyright? (That's a dumbed down example but is it really any different?) We do this type of thing in many places, we pull in header information, constants and the like - we have no choice, the code would not function otherwise.

Comment 12

[Mike Shaver (:shaver emeritus)]

If there is no other way to meaningfully express the same behaviour, then you have no option.  If you write your own code that happens to look the same, we're probably OK.

If you copy and paste from an existing code sample that is compatibly licensed, it is not OK.

Here's a rule of thumb: if it was worth copying and pasting rather than re-typing, be cautious or rewrite based on the behaviour of the code rather than the way it was entered in the page you saw.  "Work from docs, not from other code."

Constant names and other API elements are not subject to the same sort of concerns here because they are unavoidable parts of any expression of the same behaviour.  If they're not unavoidable parts, avoid them. :-)

Comment 13

[Jim Mathies [:jimm]]

I'll have a patch here in a bit. Honestly, in looking over the code I didn't follow mozilla coding standards very well. :) (This was one of the first patches I wrote when I joined though so hopefully nobody will shoot me for it.)

Comment 14

[Phil Lacy]

Jim, can you check in Bug 494989 before you start(In reply to comment #13)
> I'll have a patch here in a bit. ...

Comment 15

[Reed Loden [:reed]]

(In reply to comment #13)
> I'll have a patch here in a bit.

It's been a bit... Any update on the patch? I'd request blocking on this bug if I could...

Comment 16

[Jim Mathies [:jimm]]

(In reply to comment #15)
> (In reply to comment #13)
> > I'll have a patch here in a bit.
> 
> It's been a bit... Any update on the patch? I'd request blocking on this bug if
> I could...

Yes, planning on getting both of these rewrite bugs done this week.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

--> Core::Widget, and blocking1.9.2+

Jim, sorry to put you in the firing line on so many blockers these days, let us know how we can support you.

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> --> Core::Widget, and blocking1.9.2+
> 
> Jim, sorry to put you in the firing line on so many blockers these days, let us
> know how we can support you.

I honestly would not rec. landing this or the patch in bug 533691 on 1.9.2 this close to release. Both these patches should bake on trunk for a while.

Comment 19

[Jim Mathies [:jimm]]

Attachment: cleanup v.1

pushed to try

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 418414 [details] [diff] [review]
cleanup v.1

This removes a lot of unneeded cruft and generally reformats things. Any remaining similarities should be addressed by bug 535860.

Comment 21

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

Comment on attachment 418414 [details] [diff] [review]
cleanup v.1

Looks OK, but this is not a rewrite, right?

+      (source.dwAspect & target.dwAspect) &&
+      (source.tymed & target.tymed)) {

This code is weird, I don't understand why it's correct, although you haven't changed its functionality.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #21)
> (From update of attachment 418414 [details] [diff] [review])
> Looks OK, but this is not a rewrite, right?
> 
> +      (source.dwAspect & target.dwAspect) &&
> +      (source.tymed & target.tymed)) {
> 
> This code is weird, I don't understand why it's correct, although you haven't
> changed its functionality.

.dwAspect and .tymed consist of a bunch of bitflags that are set if the source (or target) supports a given aspect or data type.  As long as there's at least one mutually agreeable dwAspect and tymed the interaction succeeds (the source can produce data the target understands).

Comment 23

[Jim Mathies [:jimm]]

Attachment: merged to m-c

waiting on the tree to re-open.

Comment 24

[Phil Lacy]

Comment on attachment 418414 [details] [diff] [review]
cleanup v.1

> 
>-    // Break circular reference loop (see msdn)
>-    if (GetCanonicalIUnknown(pde->stgm.pUnkForRelease) ==
>-        GetCanonicalIUnknown(static_cast<IDataObject*>(this))) {
>-      pde->stgm.pUnkForRelease->Release();
>-      pde->stgm.pUnkForRelease = NULL;
>-    }
>-    return hres;

Are we still responsible releasing the object passed in here?

Comment 25

[Jim Mathies [:jimm]]

> (From update of attachment 418414 [details] [diff] [review])
> > 
> >-    // Break circular reference loop (see msdn)
> >-    if (GetCanonicalIUnknown(pde->stgm.pUnkForRelease) ==
> >-        GetCanonicalIUnknown(static_cast<IDataObject*>(this))) {
> >-      pde->stgm.pUnkForRelease->Release();
> >-      pde->stgm.pUnkForRelease = NULL;
> >-    }
> >-    return hres;
> 
> Are we still responsible releasing the object passed in here?

According to that msdn article - 

"A circular reference can occur if the client passes a STGMEDIUM structure whose pUnkForRelease member is a pointer to the data object itself. One way this can happen is if a client calls the IDataObject::GetData method, then turns around and passes the same STGMEDIUM structure to the IDataObject::SetData method"

AFAICT from testing, this never happens when we're dragging around our HGLOBALS we set for drag images.

Comment 26

[Phil Lacy]

I was worried external apps deciding to change the image or set other data.

I can just see some app using setdata to set CFSTR_PERFORMEDDROPEFFECT and pass in the dataobj for releasing. I'm not sure if that is the same thing here.

Comment 27

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/b53e720e26e6

Comment 28

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/83066d472ab1

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Return S_OK/E_UNEXPECTED from GetData, not PR_TRUE/FALSE

When retrieving arbitrary data types IDataObject::GetData passes back the return value of CopyMediumData.  CopyMediumData returns PR_TRUE and PR_FALSE to indicate whether or not it succeeded.  We shouldn't be returning PR_TRUE or PR_FALSE out of COM interface functions though (most notably because S_OK is PR_FALSE, so testing if(obj->GetData(arguments) == S_OK) will fail when the operation succeeds and vice versa).  The shell probably uses SUCCEEDED(obj->GetData()) which masks this, but according to http://msdn.microsoft.com/en-us/library/ms678431%28VS.85%29.aspx S_OK is the only success value that comes out of IDataObject::GetData.  I discovered this while writing tests for Bug 513464.

We'll want to take this patch on any branch that the original patch lands on to prevent regressions.  I used E_UNEXPECTED mostly because we never should get PR_FALSE out of that routine (unless OleDuplicateData fails) and there aren't any better fits from the values we're allowed to return per MSDN.

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 419060 [details] [diff] [review]
Return S_OK/E_UNEXPECTED from GetData, not PR_TRUE/FALSE

I'm glad you caught this, and I think it's really really cool that you caught this with a test!

Comment 31

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/12526c29c1a8

Comment 32

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2069a054e109

Comment 33

[Daniel Veditz [:dveditz]]

Gerv: is this one we need to fix on other supported branches as well (that is, 1.9.0.x and 1.9.1.x)? Trying to decide if this really does "block" the next releases.

Comment 34

[Gervase Markham [:gerv]]

Dan: it's been this way for ten years, so certainly a month or two's baking on the trunk is not unreasonable. So should we say that we should fix it on branches sometime, but it doesn't have to be the very next release?

Gerv

Comment 35

[Mike Shaver (:shaver emeritus)]

No, it's been this way for 2 years.  I think that we should bake it good, though, since breaking our users for this fix would be considerably embarrassing.

Jim: what test coverage do we have for this code?  And if this isn't a rewrite, per comment 20 and comment 21, then how is it FIXED?  The bug here is about the inappropriate use of MSFT sample code, so if we're still using it then the bug isn't fixed.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #35)
> Jim: what test coverage do we have for this code?

There's no test in the tree for this, though my patch in bug 513464 contains a test that hits this part of the code (specifically SetData and GetData for an arbitrary type).  That test passes when run against this code (after my patch from comment 29 is applied).

Comment 37

[Mike Shaver (:shaver emeritus)]

(In reply to comment #36)
> There's no test in the tree for this,

That doesn't seem right. Jim? roc?

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #35)
> Jim: what test coverage do we have for this code?

There currently is no test coverage on the drag object. Bug 535860 was filed to address that and to continue work on cleaning up the src.
 
> And if this isn't a rewrite,
> per comment 20 and comment 21, then how is it FIXED?  The bug here is about the
> inappropriate use of MSFT sample code, so if we're still using it then the bug
> isn't fixed.

I've changed the code considerably. Functionality such as that found in GlobalClone and GetCanonicalIUnknown has been replaced with newer code or removed completely. SetData, CopyMediumData, and the lookup functionality have all been updated in addition to the original changes that were made to leverage mozilla utility classes. I really don't think there's a copyright violation here, but I'm not a lawyer.

Comment 39

[Gervase Markham [:gerv]]

Jim: similarity of form is a question of patent law; commonality of origin is the realm of copyright law. As Mike says in comment 8: "Copyright doesn't protect functionality, it protects expression." If the code we still have was derived from the Microsoft code, then it's still a copyright violation. 

This is our fault for not specifying what was needed clearly enough, and not giving you any guidance on how to properly clean-room reimplement code. Let me go away and pull together the right people to work out what that is, and get back to you.

Gerv

Comment 40

[Mike Shaver (:shaver emeritus)]

No, similarity of form is a question of trademark law; similarity of method is patent.  I think we're fine here, since there is not meaningful derivation remaining.  I can check with counsel if it's necessary, though.

Thanks, Jim.

Comment 41

[Gervase Markham [:gerv]]

Yes, your first sentence is right. But the question of "what is derived?" is a question of method, not form. We clearly started by taking the Microsoft code and transformed it into what we have now (i.e. Jim changed it around rather than starting from scratch or a spec), and therefore what we have now is a derivative work of the Microsoft code.

If two people independently write two very similar movies, there is no copyright infringement (and I'm sure that's the defence James Cameron would use ;-). But if one is based on the other, there is.

Gerv

Comment 42

[harvey anderson]

A general rule to distinguish the various rights is as follows:
-copyright protects the expression of an idea as Mike described; To infringe copyright, you actually have to copy. So the ultimate defense is to have someone create the code who has not been exposed to the target code in question. This is not the only way, but it is the best practice to avoid any assertions that the code was copied. Gerv will provide more detail on this process.

-patents protect the function of an idea when implemented in real world systems and devices (there is no similarity concept in patents - either you have the same elements/functionality of all elements of a claim or not (excluding the notion of the doctrine of equivalents which is probably not appropriate for this thread);

-trademark infringement is generally determined by the "likelihood of confusion" test which is based on several factors that examine the similarity of sight, sound, and meaning of a mark, plus other factors like similarity of goods, channels, audiences, and the sophistication of the buyers.

Comment 43

[harvey anderson]

In view of Gerv's comment, the best option would be to have someone read the spec, but not the code, and then implement the code. I would not go as far as Gerv's statement that the code is derivative (nor would it be appropriate to debate that in this forum).

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Jim: can you put together branch safe patches for 1.9.0.x and 1.9.1.x and put them up for approval when reviewed and ready? Thanks!

Comment 45

[Daniel Veditz [:dveditz]]

Doesn't seem like this one is going to happen