Opened 5 years ago

Closed 4 years ago

#11650 closed bug (fixed)

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

Reported by: vidrep Owned by: korli
Priority: normal Milestone: R1
Component: Add-Ons/Translators/RTF Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #11666
Has a Patch: no Platform: All

Description

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

Attachments (5)

screenshot1.png (120.1 KB) - added by vidrep 5 years ago.
screenshot2.png (120.0 KB) - added by vidrep 5 years ago.
StyledEdit-1806-debug-21-12-2014-23-58-55.report (20.1 KB) - added by KapiX 5 years ago.
packets (336 bytes) - added by KapiX 5 years ago.
Sample file
test.zip (742 bytes) - added by bbjimmy 4 years ago.
A zip of the test file created with StyledEdit that StyledEdit cannot open.

Download all attachments as: .zip

Change History (56)

Changed 5 years ago by vidrep

Attachment: screenshot1.png added

Changed 5 years ago by vidrep

Attachment: screenshot2.png added

comment:1 Changed 5 years ago by luroh

Owner: changed from korli to pulkomandy
Status: newassigned
Summary: StyledEdit cannot open filesStyledEdit cannot open files since hrev48534

comment:2 Changed 5 years ago by vidrep

Updated to hrev48538 and now it is working again.

comment:3 Changed 5 years ago by pulkomandy

Resolution: invalid
Status: assignedclosed

comment:4 Changed 5 years ago by luroh

Resolution: invalid
Status: closedreopened

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

Last edited 5 years ago by luroh (previous) (diff)

comment:5 Changed 5 years ago by KapiX

Same here, but it crashes instead. hrev48541 gcc4h

Last edited 5 years ago by KapiX (previous) (diff)

Changed 5 years ago by KapiX

Attachment: packets added

Sample file

comment:6 Changed 5 years ago by anevilyak

Component: Applications/StyledEditAdd-Ons/Translators/RTF
Owner: changed from pulkomandy to korli
Status: reopenedassigned

comment:7 Changed 5 years ago by pulkomandy

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

comment:8 in reply to:  7 ; Changed 5 years ago by 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.

comment:9 in reply to:  8 ; Changed 5 years ago by 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.

comment:10 in reply to:  9 ; Changed 5 years ago by 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. :)

comment:11 in reply to:  10 Changed 5 years ago by 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.

Last edited 5 years ago by KapiX (previous) (diff)

comment:12 Changed 5 years ago by 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.

comment:14 Changed 5 years ago by luroh

Summary: StyledEdit cannot open files since hrev48534StyledEdit is broken on fresh (non-updated) images since hrev48534

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.

comment:15 in reply to:  14 ; Changed 5 years ago by 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.

comment:16 in reply to:  15 Changed 5 years ago by 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.

comment:17 Changed 5 years ago by 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.

comment:18 Changed 5 years ago by 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.

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by 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.

Last edited 5 years ago by luroh (previous) (diff)

comment:21 Changed 5 years ago by diver

#11174 might be related.

comment:22 Changed 5 years ago by 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?)

Last edited 5 years ago by ttcoder (previous) (diff)

comment:23 Changed 5 years ago by 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.

comment:24 Changed 5 years ago by andy-kras

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

comment:25 Changed 5 years ago by 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.

Last edited 5 years ago by vidrep (previous) (diff)

comment:26 Changed 5 years ago by vidrep

Last edited 5 years ago by vidrep (previous) (diff)

comment:27 Changed 5 years ago by KapiX

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

comment:28 Changed 5 years ago by 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

Last edited 5 years ago by vidrep (previous) (diff)

comment:29 Changed 5 years ago by luroh

Blocking: 11666 added

comment:30 in reply to:  5 Changed 5 years ago by 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.

comment:31 Changed 5 years ago by 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.

comment:32 Changed 5 years ago by luroh

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

comment:33 Changed 5 years ago by 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.

comment:34 Changed 5 years ago by 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.

Last edited 5 years ago by luroh (previous) (diff)

comment:35 Changed 5 years ago by 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.

Last edited 5 years ago by TwoFx (previous) (diff)

comment:36 Changed 5 years ago by 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?

comment:37 in reply to:  36 Changed 5 years ago by 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.

comment:38 Changed 5 years ago by luroh

Drats. Thanks KapiX.

comment:39 in reply to:  36 Changed 5 years ago by 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 :)

comment:40 Changed 5 years ago by bbjimmy

work around:

blacklist RTFTranslator

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

seems to solve the issue.

comment:41 Changed 5 years ago by pulkomandy

Resolution: fixed
Status: assignedclosed

Made the required changes in hrev48606.

comment:42 Changed 5 years ago by 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.

Last edited 5 years ago by bbjimmy (previous) (diff)

comment:43 Changed 5 years ago by bbjimmy

Resolution: fixed
Status: closedreopened

comment:44 Changed 5 years ago by luroh

Blocking: 11666 removed

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

comment:45 Changed 5 years ago by pulkomandy

Blocking: 11666 added

(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)

comment:46 Changed 4 years ago by 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.

comment:47 Changed 4 years ago by 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.

Changed 4 years ago by bbjimmy

Attachment: test.zip added

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

comment:48 in reply to:  47 Changed 4 years ago by 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?

comment:49 Changed 4 years ago by 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.

comment:50 in reply to:  40 Changed 4 years ago by 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?

comment:51 Changed 4 years ago by 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.

comment:52 Changed 4 years ago by anevilyak

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.