212090 - nsInMemoryDataSource:GetSource() doesn't iterate over assertions correctly

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Jan Varga [:janv]]

I found this bug while I was working on bug 199501.

GetSource() and GetSources() implementation should be very similar, except that
GetSource() returns only the first source.
I compared both methods in nsInMemoryDataSource and I found a problem in
GetSource().

This is how GetSources() iterates over assertions
http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsInMemoryDataSource.cpp#568
mSource is null in this case since we are actually looking for a source.
So it can be simplified to
mNextAssertion = mNextAssertion->u.as.mInvNext;

But when you look at GetSource() implementation
http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsInMemoryDataSource.cpp#1092
you found that it uses |as = as->mNext| which is in my opinion incorrect.

To be sure I put some printfs around and I found that GetSource() is not used
very often. GetSources() is used a lot. So we can assume that GetSources()
implementation is more likely correct.

I'll attach a patch.

Comment 1

[Jan Varga [:janv]]

Attachment: proposed fix

Comment 2

[jag (Peter Annema)]

Comment on attachment 127263 [details] [diff] [review]
proposed fix

Looks like you're right.

Ben, could you r= this?

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 127263 [details] [diff] [review]
proposed fix

r=[e-mail]

Comment 4

[Chris Waterson]

Seems reasonable to me. cc'ing rjc, who was probably the last person to fiddle
with this stuff.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what's this fixing, and what's the risk?  (What could it break, and how
likely is it to break?)  Could some callers be depending on the old behavior?

Comment 6

[Jan Varga [:janv]]

Say you have this kind of data in an in-memory data source:

source      | name property | host property | database property
---------------------------------------------------------------
urn:sqltest | sqltest       | localhost     | sometest

now when I call GetSource(GetResource(SQL_NS + "Name"), GetLiteral("sqltest"))
I'll get "urn:sqltest" which is correct.

if I change it like this:

source      | name property | host property | database property
---------------------------------------------------------------
urn:sqltest | sqltest       | localhost     | sqltest

I won't get anything. GetSource just doesn't work in this case.

I already landed fix for bug 199501 which depends on this bug. Without this fix
alias managemenet in SQL support is broken.

Three people agreed that this good thing to do and I didn't see any problems
with this patch.
But some weird code can always depend on the old behaviour, but only if it meets
the same kind of data.

Comment 7

[Jan Varga [:janv]]

checked in