StyledEdit is broken on fresh (non-updated) images since hrev48534

[vidrep]

hrev48535 x86_gcc2 StyledEdit does not open any text file and gives "unsupported format" error.

[vidrep]

Updated to hrev48538 and now it is working again.

[luroh]

Still present here in official nightly hrev48535 gcc2h (vmware image), and my own hrev48539 gcc2 (nightly-vmware) builds.

[KapiX]

Same here, but it crashes instead. hrev48541 gcc4h

[KapiX]

Sample file

[pulkomandy]

I can't reproduce this (including with the attached text file). hrev48541 from pkgman update.

[luroh]

Replying to pulkomandy:

I can't reproduce this (including with the attached text file). hrev48541 from pkgman update.

Both your and vidrep's comments may indicate that the problem is present only in clean installs and not in updated ones.

[KapiX]

Replying to luroh:

Both your and vidrep's comments may indicate that the problem is present only in clean installs and not in updated ones.

I am using updated install.

[luroh]

Replying to KapiX:

I am using updated install.

Ah, with the original install predating hrev48534, I presume?

I will pull a few nightlies, test, update and test again. Will also check with more than one CPU, which is what I've been using so far. Plus anything else I can think of short of calling for Diver. :)

[KapiX]

Replying to luroh:

Ah, with the original install predating hrev48534, I presume?

Yes, original install was older.

I will pull a few nightlies, test, update and test again. Will also check with more than one CPU, which is what I've been using so far. Plus anything else I can think of short of calling for Diver. :)

I have built RTFTranslator from sources and it seems to work. I am updating my install to hrev48547 to check if the problem is still present. EDIT: on hrev48547 crash does not occur.

[luroh]

After testing some official nightly vmware images, I really don't know what to say about the following results, other than that merely performing an update seems to solve the problem:

nightly 48530 - works nightly 48530 updated to 48534 - works nightly 48530 updated to 48535 - works nightly 48535 - broken, "Unsupported format" nightly 48535 updated to 48538 - works (this is consistent with vidrep's observation above) nightly 48535 updated to 48538 updated to 48541 - works nightly 48541 - broken, "Unsupported format" nightly 48541 updated to 48547 - works

As an additional supporting data point, updating my own home-brewed and broken 48545 to 48547 also fixes it.

[luroh]

FTR, not only the @nightly-* targets are affected, I see the same problem with a basic haiku-vmware-image. And a fresh 48533 works, but a fresh 48534 doesn't.

[anevilyak]

Replying to luroh:

FTR, not only the @nightly-* targets are affected, I see the same problem with a basic haiku-vmware-image. And a fresh 48533 works, but a fresh 48534 doesn't.

Out of curiosity, is that the case across multiple reboots of the same revision? To me, the inconsistent behavior + the single case of an actual crash report that we have points to an uninitialized memory/variable issue somewhere in the recent RTFTranslator changes, as that one is also responsible for handling plain and StyledEdit formats. If that's the case, then whether it works or not is pretty much going to be a matter of luck.

[KapiX]

Replying to anevilyak:

Out of curiosity, is that the case across multiple reboots of the same revision?

As for the crash, yes, it kept happening even after rebooting the system. I think that installing gcc4h and then updating it to hrev48541 would show the issue, given it is not hardware-specific.

[anevilyak]

​http://cgit.haiku-os.org/haiku/tree/src/add-ons/translators/rtf/convert.cpp#n839 looks potentially problematic; for those who can reproduce the issue, it would be interesting to know if changing that line to fileTarget->Write(rtfFile.String(), rtfFile.Length()); makes a difference.

[luroh]

Replying to anevilyak:

Out of curiosity, is that the case across multiple reboots of the same revision?

Yes, no amount of cold or warm rebooting seems to solve the problem.

Replying to anevilyak:

it would be interesting to know if changing that line to fileTarget->Write(rtfFile.String(), rtfFile.Length()); makes a difference.

No, no change here.

[anevilyak]

Possible problem found while stepping through with luroh:

in ​http://cgit.haiku-os.org/haiku/tree/src/add-ons/translators/rtf/convert.cpp#n821 , convert_plain_text_to_rtf() unconditionally casts both its source and target BPositionIO to BFile, though e.g. in the case of BTranslationUtils::GetStyledText() (what StyledEdit uses), the output is in fact a BMallocIO. This could lead to some interesting/unpredictable issues. In any case, that convert function shouldn't be making such assumptions, and especially not without checking against dynamic_cast.

[luroh]

Some interesting observations:

Nightlies 48535 and 48541 were produced by geist-bot4-ubuntu64 and are broken Nightly 48547 was produced by geist-bot3-ubuntu64 and is broken

The repos that fix the problem by updating to them were all produced by geist-bot1-ubuntu64 Updating to a revision where the repo was produced by geist-bot2-ubuntu64 (e.g. 48555) does not fix the problem. Conversely, updating a working install to such a revision will in fact break it.

So, it seems that my own host, geist-bot2,3 and 4 all produce broken builds, while geist-bot1 does not.

[diver]

#11174 might be related.

[ttcoder]

Interesting -- let's put the spotlight on pulkomandy's key comment ticket:11174#comment:20 ... since it got buried in OP messages a bit :-p /me will suspend his work on 11497 just in case the root cause is in fact the same as here.. The relation with pkgman updates is there too, that's for sure..

(edit: fixing the dangerous/outright wrong code as per Yak's comment:19 sounds quite as prioritary too.. BFile inherits from BNode first and then from BPositionIO.. a dynamic_cast would adjust the vtable pointer, but I don't think a C-style cast will?)

[luroh]

Replying to diver:

#11174 might be related.

Thanks Diver, I agree. I have cross-referenced with that ticket and it seems consistent with my findings up until 48086 where geist-bot2-ubuntu64 suddenly starts producing working repos and geist-bot1-ubuntu64 starts producing broken ones. So, there seems to be more to it than just a case of statically broken build hosts, unfortunately.

[andy-kras]

StyledEdit works good just without new RTFTranslator 0.7 add-on (if blacklisted) ;-)

[vidrep]

The last few comments brought to mind issues I have seen with Web+ as well - one day it is fast, next day so slow as to be almost unusable. There is also the mysterious audio working/not working bug #11610. I do fresh installs weekly then pkgman full-sync daily. My observation may be completely unrelated, but I felt compelled to bring it up here, just in case.

[KapiX]

Same install (hrev48547, comment 11), I just saw it happen again.

[vidrep]

Fresh install of hrev48530 - working Pkgman full-sync--->hrev48561 - working Pkgman full-sync--->hrev48562 - not working Pkgman full-sync--->hrev48563 - working Fresh install of hrev48562 - not working Pkgman full-sync--->hrev48563 - working Fresh install of hrev48565 - not working Pkgman full-sync--->hrev48569 - not working Pkgman full-sync--->hrev48571 - not working Pkgman full-sync--->hrev48574 - working Pkgman full-sync--->hrev48576 - not working

[luroh]

Replying to KapiX:

Same here, but it crashes instead. hrev48541 gcc4h

I just did a clean pure gcc4 build and I get a crash very similar to yours.

[vidrep]

I have been installing and updating 64 bit builds and checking for this bug. So far it has not manifested itself on these builds.

[luroh]

Indeed, a clean x86_64 build hrev48574 does not exhibit the problem here.

[vidrep]

I do not believe this issue is limited to only fresh installs as the ticket description implies (see comment 28 where I have been tracking installs and updates). We could try doing installs of listed "working hrev's" from my list to see if fresh installs of the same builds are broken. Note that hrev48562 is broken both as a pkgman update and as a fresh installation.

[luroh]

Already done in comment:12 (note 48535 and 48541). To complicate matters, different buildbots may have uploaded the nightlies and the repos respectively, so I don't think further basic testing of more builds will yield much.

[TwoFx]

I authored one and signed off the other commit that hrev48534 consists of. This is my analysis of the problem:

1. StyledEdit, when opening a new file, asks TranslationUtils to translate the file into styled text ​here after being called by ​this, which upon failure displays the well-known message: "Error loading \"%s\":\n\tUnsupported format".
2. TranslationUtils then does ​this call on TranslatorRoster. It is now the task of the TranslatorRoster to find the best Translator for the desired conversion and have that translate the data.
3. It does that by calling Identify on every translator it knows. For each translator that reports that it can handle thae data, it multiplies the quality and capability they report for the given input format and chooses the translator with the highest value.
4. The translator that should be chosen is of course the StyledText translator. Its ​values are 0.4 and 0.6 - resulting in a weight of 0.24.
5. However, RTFTranslator ​has the exact same values!
6. If there are multiple translators with the best value, TranslatorRoster chooses the one that it encounters earlier (like ​this), so the order that fTranslator (which is of type TranslatorMap ​= std::map) is iterated is what decides about which translator is chosen.
7. The order of the elements of a std::map is based on their keys - in this case of type translator_id ​= unsigned long.
8. Since the translators are added ​with ascending IDs, it all depends on the order in that they are added to the roster in the first place.
9. Translators are added ​directory by ​directory. Within a directory, they are added in the order that they are returned by ​GetNextRef.
10. GetNextRef ​eventually ​calls ​readdir.
11. ​I have discussed readdir order on packagefs before. Maybe someone here knows more details, but it seems unpredictable.
12. Now there are two things that can happen. Either GetNextRef mentions StyledText translator first, and everything works like a charm, or RTFTranslator gets mentioned first and gets chosen.
13. As by step two in this analysis, the next thing that happens is the ​actual translate step.
14. The best part: Although, for the translation from plain text to styled text, RTFTranslator returned B_OK on Identify, it is actually not able to perform that conversion. It instead translates the ​text to the RTF format (After all, that's what it does best, right?).
15. TranslatorRoster is completely fine with that, but TranslationUtils looks for the nonexistent STXT header and therefore returns ​B_BAD_TYPE, which causes StyledEditView to return ​B_BAD_TYPE, which causes StyledEditWindow to ​display the error message.

This explains why the error only happens sometimes - depending on the order that readdir reads the files, either StyledText translator does the right thing, or RTFTranslator does the wrong thing. The fact that updating changes the behavior might be a hint that the order in which the files are read is decided when the package is created. Note: I have not verified this, but I assume that the DataTranslations window is populated in the same way (the fact that, when putting a translator into /boot/home/config/non-packaged/Translators/, it always appears above every other translator supports this theory, as it shows the directory-by-directory order from step 9). That would mean that, from the order in which StyledEdit translator and RTFTranslator appear in the window, it is possible to predict whether StyledEdit will work or not. Just an idea though, I didn't test this. If that is in fact the problem, it is rather easy to fix, just don't let RTFTranslator accept translations that it cannot perform!

...
            sizeof(info->name));
        strcpy(info->MIME, "text/plain");
        }
+    if (outType != RTF_TEXT_FORMAT)
+        return B_NO_TRANSLATOR;
}
return B_OK;
...

Adding those two lines (around line 200 of RTFTranslator.cpp) and recompiling fixed the problem for me. Of course this may just be chance and it did not fix the problem at all, but it seems like a reasonable explaination to the phenomenon that was observed.

[luroh]

TwoFx: thank you for the analysis, if you are correct you will be rewarded ...before being punished. ;-)

I have a question. Since the selection behavior seems to be static within a build, would it be correct to assume that files intended to be processed by STXTTranslator and RTFTranslator respectively would be equally but oppositely affected in what we up until now have called 'broken' and 'working' builds? And if so, what would be a suitable RTF file to test with?

[KapiX]

Replying to luroh:

I have a question. Since the selection behavior seems to be static within a build

It is not. I recently saw the crash on the same install that worked earlier.

[luroh]

Drats. Thanks KapiX.

[TwoFx]

Replying to luroh:

TwoFx: thank you for the analysis, if you are correct you will be rewarded ...before being punished. ;-)

I'll happily forward all complaints to the GCI student from last year who wrote the broken code - or the GCI mentor who told me to base my work on it :)

[bbjimmy]

work around:

blacklist RTFTranslator

copy RTFTranslator to /boot/system/non-packaged/add-ons/Translators

seems to solve the issue.

[pulkomandy]

Made the required changes in hrev48606.

[bbjimmy]

On hrev 48606:

it is working much better, but still gives the "Unsupported fotmat" error if the file is empty or has one short line of text.

[luroh]

(In #11666) Removing #11650 as that problem is now well understood. Adding #11610.

[pulkomandy]

(In #11666) There is always an actual bug somewhere behind these issues. The fact that the actual bug is understood does not suddenly make the issues consistent and reproductible, so they should still be referenced here, and we should look for a way to make them consistent and reproductible (for example by making sure we always populate packages in the same order)

[vidrep]

I have done several fresh installs and updates over the past week or more, and I have not seen this issue reoccur in any of the recent builds.

[bbjimmy]

The issue is still here in hrev48698.

To duplicate my results:

Right click on the desktop, and select New text file. Rename the file to something more interesting than "New text file" and double click.

I have a file named test, with ine line of text "test" created by StyledEdit that cannot be opened in StyledEdit.

[bbjimmy]

A zip of the test file created with StyledEdit that StyledEdit cannot open.

[TwoFx]

Replying to bbjimmy:

The issue is still here in hrev48698.

To duplicate my results

hrev48694. I cannot reproduce the error by downloading your file or following your instructions and have not had any issues since 48606. Did you install any translators aside from the standard roster?

[bbjimmy]

I have not added any translators that I know of. This issue started with the translator changes in hrev48534, and has been persistant since.

[TwoFx]

Replying to bbjimmy:

work around:

blacklist RTFTranslator

copy RTFTranslator to /boot/system/non-packaged/add-ons/Translators

seems to solve the issue.

Does this still work for you?

[bbjimmy]

my mistake, I found an old version of RTFTranslatorn in /boot/system/non-packaged/add-ons/Translators removing this solved the issue. I think this ticket can be closed now. Sorry for the noise.