Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#1850 closed bug (fixed)

wrong content in profiles

Reported by: globetrotter_tt Owned by: lantti
Priority: blocker Milestone: 1.3
Component: BW Profile Keywords: profile, translation,
Cc: planetcruiser, jsfan, shevek, jeanyves

Description

Issue:

  • A couple of members reported that they have content in their profiles, that they have not entered. Additionally they are not able to modify or delete content in their own profiles

Clues:

  • This happened only in profiles which were filled out in more than one language.
  • You can see the other content when you change the language in the footer
  • Then you can see a link of that language on the top right in the profile, even if the user has not created a translation for this language.

Profiles:

  • If you need to get the name of the profiles where this happened, please send me a PM

Attachments (1)

patch1850.diff (5.9 KB) - added by shevek 6 years ago.
Patch 1850

Download all attachments as: .zip

Change History (57)

comment:1 Changed 6 years ago by shevek

@globetrotter_tt: I can't see any language for the last problematic profile.

comment:2 Changed 6 years ago by abyssin

The problem might be related to this bug: http://trac.bewelcome.org/ticket/1691.

comment:3 Changed 6 years ago by planetcruiser

i will look into this and can solve this until 1.3 is released. :)

release coordinators, feel free to attach to 1.3

comment:4 Changed 6 years ago by shevek

  • Milestone changed from unassigned to 1.3
  • Owner set to planetcruiser
  • Status changed from new to assigned

Thanks, planetcruiser. Set milestone and assigned to you.

comment:5 Changed 6 years ago by lantti

  • Cc antti.siponen@… added

comment:6 Changed 6 years ago by planetcruiser

lantti, do you want to take this one? you seem keen. ;) i can run some queries on the live db if you need background info.

a wild guess would be that the bug has something to do with the "crypted".. let's call it obfuscation. maybe a crypted id confused with a member id or something silly like that?

i would check which member the wrong profile information is coming from and see what this member has in common with the profile their information appears on. compare member ids, address ids, crypted ids etc. there must be a pattern somewhere. 8-)

to reproduce the bug locally i would first activate a separate "crypted" db (see config for www and alpha), then create a profile with several translations and then go wild on it until the bug is triggered.

comment:7 Changed 6 years ago by lantti

  • Cc antti.siponen@… removed
  • Owner changed from planetcruiser to lantti
  • Status changed from assigned to accepted

comment:8 Changed 6 years ago by jsfan

Sent lantti some info he asked for. Just to have a record here. :)

comment:9 Changed 6 years ago by lantti

From the data jsfan sent is is clear why the wrong data is displayed and why the user can't change his text. He was somehow given an already existing id for his new text and now he can create and modify only such versions of that text that original didn't have because he is not the owner of the other versions. I'd expect the other user (the owner of the original) would have similar problems now if she tried to change her text in the laguages that have now a version from the other user. I'll start figuring out how we got there...

comment:10 Changed 6 years ago by lantti

Yep, she has. Just go to her profile and view it in French and you will have text from the profile summary of the other users profile appearing there in her comment about CouchSurfing? group.

comment:11 Changed 6 years ago by lantti

I don't know if that is the reason for this problem, but the id generation in InsertInMTrad function (modules/i18n/lib/words.lib.php) that is used to store these texts has a race condition. The code for generating a new id is simply:

$s = $this->_dao->query("Select max(IdTrad?) as maxi from memberstrads"); if (!$s) {

throw new PException('Failed in InsertInMTrad searching Next max IdTrad?');

} $rr=$s->fetch(PDB::FETCH_OBJ) ;

if (isset ($rr->maxi)) {

$IdTrad? = $rr->maxi + 1;

} else {

$IdTrad? = 1;

}

After that some time passes. Some more time passes. Civilizations rise and fall. Continents change shape. And finally a insert query to the database is executed. I'd think there is plenty of opportunity here for a second text insert getting the same id. I'll still need to check some timestamps and logs to see if this could be the case here.

comment:12 Changed 6 years ago by lantti

While I think the race condition mentioned in the previous comment should get fixed too I think it was not the cause this time. All the same the cause seems to be the faulty generation and management of ids for the texts. I have managed to reliably reproduce this issue on a local install now using the following procedure: Create a new account, login on that account, write a profile summary, delete all translations of the profile (should be only one, english), logout, login on an another account, create a new piece of translatable profile text (for example join a new group and write a group join comment). The new text written should have appeared on the profile summary of the other profile. This happens because deleting the last translation of a text allows the text id to be reused (see the id generation algorithm in the previous post), but none of the references to the deleted text anywhere are removed or updated.

comment:13 Changed 6 years ago by shevek

Any news on this topic? Would like to see this in 1.3...

comment:14 Changed 6 years ago by lantti

I found those two ways above to create overlapping profiles and I'm thinking about ways to fix them. Still the revision history for the reported profiles doesn't look like they could have got mixed in either of these ways so there is still more wrong (or I'm just missing something). Anyway the way to generate or manage IdTrad? values here is in no way sane and the same code seems to be copy-pasted in the forums code too. If we want a quick fix I have thought about two options:

a) Just removing any references to invalid IdTrad? values when deleting text (following a back link in the translation record that tells where the text was used). This creates concurrency issues and and for example would not work in a case where the ids are already mixed like now and the back link is not anymore sane.

b) Never deleting a last translation of a text and leaving a blank translation there instead. This would leave IdTrad? values valid even when all text is deleted and also prevent re-use of an IdTrad?. The downside is that this way a text that is once written can never again return to the original non-existent state (IdTrad? = 0) I have no idea if the existence of a text is used to flag something somewhere...

I'll keep reading the code in hope of clarifying some of this or getting more ideas. If someone with sql experience wants to take over, feel free to do so. This is the first project ever where I deal with sql databases...

Last edited 6 years ago by lantti (previous) (diff)

comment:15 Changed 6 years ago by lantti

Fixing the damage already done to the live database is another concern also, but that needs to be done by the db admins directly I think.

comment:16 Changed 6 years ago by shevek

Main focus should be to keep it from happening again.

Fixing the damage already done would mean that there is a way to detect the problem within the DB. Than we probably should inform the affected users to fix it themselves as soon as we know it won't happen again.

comment:17 Changed 6 years ago by shevek

While I tried to figure out what's going wrong related to #1707 I tried to delete Sitarane's french profile translation in the test db. That throw an exception 'forum::MakeRevision? fail to select id=# from memberstrads' maybe that helps investigating?

comment:18 Changed 6 years ago by shevek

Any chance we can get this in till tomorrow?

comment:19 Changed 6 years ago by lantti

Depends what "this" means. I have a fix for the id reuse after delete (I took the option b) there above), but poorly tested and definitely needs hard review before deployed on alpha. Plus I'm not all sure that this was the cause of the problem reported.

comment:20 Changed 6 years ago by lantti

Ok, let's take that back. The option b) also has a concurrency issue as it needs to know if it is deleting the last translation or not. The fix I have doesn't fix anything, just move the issue...

comment:21 Changed 6 years ago by planetcruiser

if i may interfere.. this is a grave bug (hence the blocker status) and we should definitively fix it with 1.3. as i understand it the release date is still 2 weeks away, right?

lantti, need help with it? is there any code to review or test locally? a feature branch in your clone maybe?

comment:22 Changed 6 years ago by lantti

Ok, ok. I pushed the fix I made. It does fix the profile text getting mixed in the way I described above, but it causes timing issues that can lead the profiles getting mixed all the same. I'm sure there are simple sql idioms to fix all this, but I'm just not aware of any.

Last edited 6 years ago by lantti (previous) (diff)

comment:23 Changed 6 years ago by shevek

I asked as we set a dead line on the dev list to ensure proper testing (which probably should be done locally as this might interfere with the production system).

Lantii where did you push the fix to? I can't see it in the list of commits to develop and would definitely like to review the patch.

comment:25 Changed 6 years ago by shevek

Okay, found it. So the fix is in words.lib.php, now I understand why you want thorough testing.

Checked the code. Looks good to me. Could we add TABLE_LOCks to avoid write access to the table while we fix the IdTrads??

Last edited 6 years ago by shevek (previous) (diff)

comment:26 follow-up: Changed 6 years ago by planetcruiser

lantti: how can we test this locally? sounds like this is a random bug. do you have some reproduce steps? maybe with commenting out lines somewhere or modifying something in the db temporarily?

comment:27 in reply to: ↑ 24 Changed 6 years ago by planetcruiser

Replying to lantti:

https://gitorious.org/bewelcome/rox/commit/66f66553a65ba44867d5c2f07339e835f23c139c

oh, and is this safe to deploy to alpha? if not, what should we be careful with?

comment:28 in reply to: ↑ 26 Changed 6 years ago by shevek

lantti: how can we test this locally? sounds like this is a random bug. do you have some reproduce steps? maybe with commenting out lines somewhere or modifying something in the db temporarily?

See comment 14. And sitarane's profile in the test DB might already have the issue.

comment:29 Changed 6 years ago by shevek

If you delete the last profile language the value for ProfileSummary? is kept instead of set to 0 (or -1). Does anyone know if that's intentional?

comment:30 Changed 6 years ago by lantti

planetcruiser: I was a bit reluctant to push the fix because it doesn't really represent the idea how I'd like to see things fixed in general. I chose the option b) (see comment 14) because I thought I could do it in a "safe" way (no race conditions), but it turned out not to be the case. I'd prefer not to see this fix deployed anywhere, but I pushed it to get the discussion here a bit more concrete. That said, it does seem to fix the issue here, but is far far from a clean fix.

shevek: Ideally when the last translation for a text is deleted the references to it (for example the value of ProfileSummary?) should be set to 0. That is the value used initially to show missing text. I believe that dangling references like we have now are not left there intentionally. How it works now is that encountering a dangling reference while displaying text is logged as a bug, but then a blank translation is returned. So zeroing the references while deleting the last translation should be safe (and was my option a) there above) if done in a "safe" way. I just don't know how to accomplish that because for members and memberstrads tables we are using MyISAM which doesn't support transactions. (blafaselblubb sent me an interesting idea, I'll have to look into it. It depends on never deleting anything and on the first glance looks "safe")

shevek: The problem with sitaranes profile could or could not be related. All I found out so far is that something somewhere calls delete with a null IdTrad?.

comment:31 Changed 6 years ago by shevek

@lantti: See the result of that suggestion above.

That solution just reuses the former trad id as still set in the members table. That should be safe as there is a hole at that point in the list of trad ids (as the original one was changed to -tradid).

Last edited 6 years ago by shevek (previous) (diff)

comment:32 Changed 6 years ago by lantti

Looks very good, this was more or less what blafaselblubb was suggesting too. Do you think we could also get rid of the select query in deleteMTrad? I don't see that we are actually using the result for anything. The exclusive locking in the InsertMTrad is something that I'm not at all competent to comment on because I have no idea of the actual semantics of such locks, but do we know that they are good to use? They don't deadlock on network glitches or anything? If they are good then your patch looks good to me. This also preserves more of the original (and completety silly) behaviour of the delete in case something somewhere depends on it.

Also sorry for the slow communication, I'll try to get more responsive in couple of days.

Changed 6 years ago by shevek

Patch 1850

comment:33 Changed 6 years ago by shevek

I'd leave the select query there as it checks if there is a mismatch in the DB between owner and current member. That's actually what happens at the moment. An IdTrad? is reused by another member. If we allow to delete that someone else's profile might look weird.

The patch updates the select query so that it really should return only a result for the current language.

I updated the attached patch so that the revision is only done after it is clear that the delete will happen and fixed #1857 at the same time as the necessary information comes up as a result of the query.

comment:34 Changed 6 years ago by lantti

Sorry for the long comment. I tested Sheveks patch locally. There was a minor mistake with the call to MakeRevision?, but that is easy to fix. The id to pass as the first parameter should be $Trad->id and not $s->id (which doesn't exist, as $s is the db statement object, not the db row object). Unfortunately there seems to be a little rox surprise again. While testing the delete function, I noticed that now the MakeRevision? function started failing instead. Before it didn't crash regularly because the id passed to it was more or less random garbage and often coincided with an existing record somewhere. Now it fails always when deleting translations from a relatively new profile which has not all the fields filled in yet and still have text fields with IdTrad? '0'. The current implementation will try to delete translations from the IdTrad? '0' all the same and the check for inexistent records is done in the deleteMTrad function. The problem seems to be that this check doesn't work at all and never did. This check is done by checking the validity (non-nullity) of the statement object returned by the db query call and it is valid for all successful queries including those that return an empty set. So in the case deleteMTrad is called for an inexisting translation then MakeRevision? will be called for an inexisting record and fail. Curiously there is a check for such case also in the MakeRevision? function, but also this check is broken, just in a slightly different way. Instead of failing to notice the error it crashes on the error...

comment:35 follow-up: Changed 6 years ago by shevek

Great stuff obviously ;-)

deleteMTrad is called exactly once. Could we just prevent the call with TradId? = 0?

A question to planetcruiser or jsfan: Do we have an archive DB setup in syshcvol?

comment:36 Changed 6 years ago by lantti

I pushed Sheveks patch with my modifications to fix the call to MakeRevision? and the the check for inexistent record in deleteMTrad. Commit 69ad9ff2e30cf6e2538a9cfbb2d95151cd9607f7. As this thing gets more and more weird and we are still playing directly with the members data, I'd like that several other people would review and test that commit...

comment:37 in reply to: ↑ 35 ; follow-up: Changed 6 years ago by planetcruiser

Replying to shevek:

A question to planetcruiser or jsfan: Do we have an archive DB setup in syshcvol?

i don't know what that means, sorry. ;) where could i check that? rox_local.ini for www looks like this:

[db]
dsn = "mysql:host=localhost;dbname=BW_MAIN"
user = "bewelcome"
password = "***"

[phpflickr]
tmpfolder = "/tmp"

[syshcvol]
paypal_authtoken = "***"
MailToNotifyWhenNewMemberSignup = "***"
EncKey = "***"
SiteName = "www.bewelcome.org"
Crypted = "BW_CRYPTED."
MessageSenderMail = "message@bewelcome.org"

[env]
sitename = "BeWelcome"
baseuri = "http://www.bewelcome.org/"
baseuri_https = "https://www.bewelcome.org/"
suspend_url = "http://www.bewelcome.org/tb/suspendinfo.html"

also: i lost overview a bit, could you point me to the commits that i should still review? everything that has "1850" in it or are some of them obsolete by now?

comment:38 Changed 6 years ago by planetcruiser

from shevek via dev list:

[this ticket] alters content of the databse in a way that might interfere witrh the current code used on www.

could we not work around this by running a script that repairs historical damage on the live database only once? think hot-fixing! :)

all the code for this ticket then needs to do is to make sure that no future corruptions appear.

comment:39 in reply to: ↑ 37 Changed 6 years ago by shevek

A question to planetcruiser or jsfan: Do we have an archive DB setup in syshcvol?

i don't know what that means, sorry. ;) where could i check that? rox_local.ini for www looks like this:

From MakeRevision? in words.lib.php

$str = "INSERT INTO " . $_SYSHCVOL['ARCH_DB'] . ".previousversion(IdMember,TableName,IdInTable,XmlOldVersion,Type) VALUES(" . $IdMember . ",'" . $TableName . "'," . $Id . ",'" . mysql_real_escape_string($XMLstr) . "','" . $DoneBy . "')";

also: i lost overview a bit, could you point me to the commits that i should still

review? everything that has "1850" in it or are some of them obsolete by now?

Just the last commit would do.

Regarding hotfix: that will be difficult. We could generate a list where owner and TradId? don't match. But how do we decide who gets the content?

comment:40 Changed 6 years ago by planetcruiser

regarding the archive db, it looks like this on the live server:

mysql> show tables;
+-------------------+
| Tables_in_BW_ARCH |
+-------------------+
| logs              |
| oldest_logs       |
| oldvisits         |
| previousversion   |
| tata              |
+-------------------+
5 rows in set (0.00 sec)

whatever.. let's just ignore it for being some obscure logging mechanism.

Just the last commit would do.

gitorious changeset link please. :) sorry, i don't follow rox commits so closely these days.

But how do we decide who gets the content?

i don't exactly understand who has whose wrong content at the moment. general rule: if we can not resolve the owner of corrupted data, the data should be deleted. our faulty software is to blame for the data loss and we need to communicate this well to the affected users.

if this does not answer your question, could you supply an example of which data ends up where, or point me to a comment where this is already explained?

comment:41 Changed 6 years ago by shevek

tata looks fun ;)

I always add my commits, just forgot that I didn't push this one: https://gitorious.org/bewelcome/rox/commit/4e1485049b2373065f20d24de5a2942b296293a0

I don't have an example, but lantti does or jsfan :-)

My fear is that we might need to delete a lot of data. Any of these columns might contain faulty data:

            'Occupation', 
            'ILiveWith',  
            'MaxLenghtOfStay',
            'MotivationForHospitality',
            'Offer',
            'Organizations',
            'AdditionalAccomodationInfo',
            'OtherRestrictions',
            'InformationToGuest',
            'Hobbies',
            'Books',
            'Music',
            'Movies',
            'PleaseBring',
            'OfferGuests',
            'OfferHosts',
            'PublicTransport',
            'PastTrips',
            'PlannedTrips',
            'ProfileSummary'

This query should reveal all problems for Occupation:

SELECT DISTINCT
    m1.username, m2.username, t.IdTrad
FROM 
  members as m1, members as m2, memberstrads as t
WHERE
    m1.Occupation = t.IdTrad
    AND m1.id <> t.IdOwner
    AND m2.id = t.IdOwner

comment:42 Changed 6 years ago by globetrotter_tt

comment:43 Changed 6 years ago by shevek

I just realized that we actually don't have to delete any data. It is enough to reset the field in members to 0 for all ids that point to a different owner in memberstrads. This done the users with problems should be able to update everything again.

comment:44 Changed 6 years ago by lantti

Are you sure we don't have a situation where the same text could legally have different owners for different translations of the text? I can't say that I understand this system any better, but wouldn't that happen for example if a group has several admins and they translate the group description to different languages? Just an idea, that popped into my mind. I don't know if this can actually happen.

comment:45 Changed 6 years ago by shevek

If different admins edit the group description they are owners of the respective description. If both edit the Spanish description you get two rows in the DB. Which is obviously nonsense (it always uses the first found one).

If you can't edit a profile item because you're not the owner the reset of the link to the MTradId to 0 fixes this.

If your group description is in fact a profile item of someone else the reset of the group link to MTradId to 0 would fix that as well.

So additionally to what I said above we need to take a look at the groups table to figure which rows need to be reset. (But we haven't yet gotten any complaint there I believe.)

Last edited 6 years ago by shevek (previous) (diff)

comment:46 Changed 6 years ago by jsfan

Deployed on alpha.

comment:47 Changed 6 years ago by crumbking

how to test here?

comment:48 Changed 6 years ago by shevek

@crumbking: see comment 14 by lantii. You just need to ensure you delete the last profile content and afterwards create a new MTrad like a group description. Check the Db afterwards for correct number handling.

comment:49 in reply to: ↑ description Changed 6 years ago by crumbking

Replying to globetrotter_tt:

Issue:

  • A couple of members reported that they have content in their profiles, that they have not entered. Additionally they are not able to modify or delete content in their own profiles

I would say BW admin should check those profiles on alpha now for:

  • does they still see other content
  • are they able to modify or delete content in their own profiles

comment:50 Changed 6 years ago by shevek

Unfortunately the content can't be changed even after the fix. The fix only ensures that the same doesn't happen again. I already have a list of affected profiles. So we might send a newsletter to these people.

comment:51 Changed 6 years ago by shevek

Live database has an index on IdTrad?, IdOwner? and IdLanguage? that isn't set in the test DB. Therefore I introduced an additional DELETE just in case.

Discussed with Christian and committed: https://gitorious.org/bewelcome/rox/commit/9b46e5e7ffb2fc4fcd12719583cd4678537d6de5

comment:52 Changed 6 years ago by jsfan

  • Resolution set to fixed
  • Status changed from accepted to closed

Works. Closing.

comment:53 Changed 6 years ago by planetcruiser

so, do we still need to inform users? i or christian can generate an updated list of affected profiles. i included my queries in the email to shevek that i sent 8 days ago.

comment:54 Changed 6 years ago by shevek

I'd say we need to do that. I discuss with Christian what a feasible way to solve this is and post it here afterwards.

comment:55 Changed 6 years ago by shevek

We have 61 affected profiles (if we ignore the AskToLeave? and TakenOut? ones). To decide which text belongs to whom can't be done automatically (if you check for a mismatching owner all entries appear twice).

Christian is pretty sure we can get the information on member level and inform them via mailbot.

Cleanup should then fix the members table by setting the affected trad ids to 0. Enabling the people to edit there profile again.

comment:56 Changed 6 years ago by planetcruiser

sounds like a plan. let's do this. new ticket?

could we document the queries that were and that are run on the live db for this issue somewhere? maybe as comments in this ticket?

there were other queries on the live db as well, right? please post them to the tickets so others can review.

Note: See TracTickets for help on using tickets.