Bug 118029 - crashtesting ooo24656-1.doc with --convert-to pdf
Summary: crashtesting ooo24656-1.doc with --convert-to pdf
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.2.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.2.0
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-06 11:04 UTC by Noel Grandin
Modified: 2018-06-07 06:22 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Noel Grandin 2018-06-06 11:04:22 UTC
Description:
This crashes sinces my Bitmap->BitmapEx changes

Actual Results:  
xxx

Expected Results:
xxx


Reproducible: Always


User Profile Reset: No



Additional Info:


User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.181 Safari/537.36
Comment 1 Noel Grandin 2018-06-06 11:04:49 UTC
bt from crash:
#0  0x00007ffff784d428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff784f02a in __GI_abort () at abort.c:89
#2  0x00007ffff788f7ea in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff79a8ed8 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff789837a in malloc_printerr (ar_ptr=<optimized out>, ptr=<optimized out>, str=0x7ffff79a8fa0 "double free or corruption (fasttop)", action=3) at malloc.c:5006
#4  _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3867
#5  0x00007ffff789c53c in __GI___libc_free (mem=<optimized out>) at malloc.c:2968
#6  0x00007fffeee1d5b0 in o3tl::cow_wrapper<MapMode::ImplMapMode, o3tl::UnsafeRefCountingPolicy>::release (this=0x7fffb00010d0) at /home/noel/libo/include/o3tl/cow_wrapper.hxx:205
#7  0x00007fffeee1d3e5 in o3tl::cow_wrapper<MapMode::ImplMapMode, o3tl::UnsafeRefCountingPolicy>::~cow_wrapper (this=0x7fffb00010d0) at /home/noel/libo/include/o3tl/cow_wrapper.hxx:248
#8  0x00007fffeee1cc45 in MapMode::~MapMode (this=0x7fffb00010d0) at /home/noel/libo/vcl/source/gdi/mapmod.cxx:94
#9  0x00007fffef03e471 in Bitmap::~Bitmap (this=0x7fffb00010b8) at /home/noel/libo/vcl/source/bitmap/bitmap.cxx:114
#10 0x00007fffeed71da2 in BitmapInfoAccess::~BitmapInfoAccess (this=0x7fffb00010b0) at /home/noel/libo/vcl/source/gdi/bmpacc.cxx:80
#11 0x00007fffeed723e5 in BitmapReadAccess::~BitmapReadAccess (this=0x7fffb00010b0) at /home/noel/libo/vcl/source/gdi/bmpacc.cxx:112
#12 0x00007fffeed72409 in BitmapReadAccess::~BitmapReadAccess (this=0x7fffb00010b0) at /home/noel/libo/vcl/source/gdi/bmpacc.cxx:111
#13 0x00007fffef03fb0b in Bitmap::ReleaseAccess (pBitmapAccess=0x7fffb00010b0) at /home/noel/libo/vcl/source/bitmap/bitmap.cxx:389
#14 0x00007fffeecde94a in vcl::ScopedBitmapAccess<BitmapReadAccess, Bitmap, &Bitmap::AcquireReadAccess>::~ScopedBitmapAccess (this=0x7fffbfb8ba50)
    at /home/noel/libo/include/vcl/scopedbitmapaccess.hxx:85
#15 0x00007fffeed64c42 in BitmapEx::GetPixelColor (this=0x7fffb00015e8, nX=116, nY=48) at /home/noel/libo/vcl/source/gdi/bitmapex.cxx:759
#16 0x00007fffe8d1bf92 in drawinglayer::texture::GeoTexSvxBitmapEx::modifyBColor (this=0x7fffb00015e0, rUV=..., rBColor=..., rfOpacity=@0x7fffbfb8bd30: 1)
    at /home/noel/libo/drawinglayer/source/texture/texture3d.cxx:125
#17 0x00007fffe8d1c848 in drawinglayer::texture::GeoTexSvxBitmapExTiled::modifyBColor (this=0x7fffb00015e0, rUV=..., rBColor=..., rfOpacity=@0x7fffbfb8bd30: 1)
    at /home/noel/libo/drawinglayer/source/texture/texture3d.cxx:241
#18 0x00007fffe8d11c74 in ZBufferRasterConverter3D::decideColorAndOpacity (this=0x38abaf0, rColor=...) at /home/noel/libo/drawinglayer/source/processor3d/zbufferprocessor3d.cxx:120
#19 0x00007fffe8d0f307 in ZBufferRasterConverter3D::processLineSpan (this=0x38abaf0, rA=..., rB=..., nLine=65, nSpanCount=0)
    at /home/noel/libo/drawinglayer/source/processor3d/zbufferprocessor3d.cxx:309
#20 0x00007fffe9ca6e0d in basegfx::RasterConverter3D::rasterconvertB3DArea (this=0x38abaf0, nStartLine=0, nStopLine=177) at /home/noel/libo/basegfx/source/raster/rasterconvert3d.cxx:130
#21 0x00007fffe9ca7bd5 in basegfx::RasterConverter3D::rasterconvertB3DPolyPolygon (this=0x38abaf0, rFill=..., pViewToEye=0x38a6340, nStartLine=0, nStopLine=177)
    at /home/noel/libo/basegfx/source/raster/rasterconvert3d.cxx:321
#22 0x00007fffe8d0fdd9 in drawinglayer::processor3d::ZBufferProcessor3D::rasterconvertB3DPolyPolygon (this=0x38a6290, rMaterial=..., rFill=...)
    at /home/noel/libo/drawinglayer/source/processor3d/zbufferprocessor3d.cxx:513
#23 0x00007fffe8d096d8 in drawinglayer::processor3d::DefaultProcessor3D::impRenderPolyPolygonMaterialPrimitive3D (this=0x38a6290, rPrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:479
#24 0x00007fffe8d09bb0 in drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D (this=0x38a6290, rBasePrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:570
#25 0x00007fffe8d0446b in drawinglayer::processor3d::BaseProcessor3D::process (this=0x38a6290, rSource=...) at /home/noel/libo/drawinglayer/source/processor3d/baseprocessor3d.cxx:62
#26 0x00007fffe8d085e3 in drawinglayer::processor3d::DefaultProcessor3D::impRenderBitmapTexturePrimitive3D (this=0x38a6290, rPrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:294
#27 0x00007fffe8d09ae8 in drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D (this=0x38a6290, rBasePrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:539
#28 0x00007fffe8d0446b in drawinglayer::processor3d::BaseProcessor3D::process (this=0x38a6290, rSource=...) at /home/noel/libo/drawinglayer/source/processor3d/baseprocessor3d.cxx:62
#29 0x00007fffe8d09c28 in drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D (this=0x38a6290, rBasePrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:582
#30 0x00007fffe8d0446b in drawinglayer::processor3d::BaseProcessor3D::process (this=0x38a6290, rSource=...) at /home/noel/libo/drawinglayer/source/processor3d/baseprocessor3d.cxx:62
#31 0x00007fffe8d098c3 in drawinglayer::processor3d::DefaultProcessor3D::impRenderTransformPrimitive3D (this=0x38a6290, rTransformCandidate=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:499
#32 0x00007fffe8d09bcb in drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D (this=0x38a6290, rBasePrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:576
#33 0x00007fffe8d0446b in drawinglayer::processor3d::BaseProcessor3D::process (this=0x38a6290, rSource=...) at /home/noel/libo/drawinglayer/source/processor3d/baseprocessor3d.cxx:62
#34 0x00007fffe8d098c3 in drawinglayer::processor3d::DefaultProcessor3D::impRenderTransformPrimitive3D (this=0x38a6290, rTransformCandidate=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:499
#35 0x00007fffe8d09bcb in drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D (this=0x38a6290, rBasePrimitive=...)
    at /home/noel/libo/drawinglayer/source/processor3d/defaultprocessor3d.cxx:576
#36 0x00007fffe8d0446b in drawinglayer::processor3d::BaseProcessor3D::process (this=0x38a6290, rSource=...) at /home/noel/libo/drawinglayer/source/processor3d/baseprocessor3d.cxx:62
#37 0x00007fffe8c6a3d3 in drawinglayer::primitive2d::ScenePrimitive2D::create2DDecomposition(drawinglayer::primitive2d::Primitive2DContainer&, drawinglayer::geometry::ViewInformation2D const&) const::Executor::doWork() (this=0x372d240) at /home/noel/libo/drawinglayer/source/primitive2d/sceneprimitive2d.cxx:405
#38 0x00007ffff4e13923 in comphelper::ThreadTask::execAndDelete (this=0x372d240) at /home/noel/libo/comphelper/source/misc/threadpool.cxx:263
#39 0x00007ffff4e16a8f in comphelper::ThreadPool::ThreadWorker::execute (this=0x33a44d0) at /home/noel/libo/comphelper/source/misc/threadpool.cxx:70
Comment 2 Noel Grandin 2018-06-06 11:05:37 UTC
See
   https://gerrit.libreoffice.org/#/c/55366
Comment 3 Noel Grandin 2018-06-06 11:09:58 UTC
The cause of this seems to be:

    commit 63e65d1743264dfa26d2aba615d71978e65784e8
    Author: Noel Grandin <noel.grandin@collabora.co.uk>
    Date:   Wed May 30 11:11:21 2018 +0200
    dont use GetMask in GeoTexSvxBitmapEx
    
    Reviewed-on: https://gerrit.libreoffice.org/55050

And the reason is that (as noted by caolan) previously we created the ScopedReadAccess stuff before we entered the multi-threaded part.
Comment 4 Noel Grandin 2018-06-06 11:11:03 UTC
IRC discussion:

<noelgrandin> caolan__, I am looking at that Bitmap crashtesting report
<loircbot> LibreOffice (online) fitojb * favicon.ico: Favicon refresh
<caolan__> noelgrandin, thanks, looks like it used to work cause the BitmapReadAccess was taken in the ctor originally before the threading kicked in
<noelgrandin> caolan__, ah, interesiting. Currently I am trying to make BitmapInfoAccess hold the underlying SalBitmap rather than the Bitmap, do you have an easier fix?
<noelgrandin> caolan__, part of the threading problem is also that MapMode is a non-threadsafe COW thingy
<caolan__> noelgrandin, I'm not sure what the Bitmap vision is, is BitmapReadAccess to go away ?
<noelgrandin> caolan__, and MapMode is part of Bitmap, hence my idea to bypass Bitmap
<noelgrandin> caolan__, the vision (AFAIK) is to dump Bitmap, and have BitmapEx hold SalBitmap
<noelgrandin> and have almost everything bitmap related (other than BitmapEx) hidden inside vcl
<noelgrandin> mmeeks, quikee ^^^
<quikee> the vision is to merge Bitmap and BitmapEx into BitmapEx
<mmeeks> caolan__: the big plan is to get a single Alpha bitmap (and drawing surface) - so we don't render everything twice in alpha mode at huge cost =)
<thorsten> noelgrandin: uh - Bitmap itself is COW, so why is mapmode making any difference? ;)
<caolan__> well, I guess if there was a PixelGetter or whatever which was created where BitmapReadAccess used to be created, and use used where the GetPixel is called, then the problem wouldn't arise
<mmeeks> caolan__: step #1 - bring all the separate pixel-bashing madness into VCL so we can surround & kill it ;-)
<noelgrandin> thorsten, because, in this case, we are accessing Bitmap via multiple threads, and Bitmap holds MapMode, and MapMode is not threadsafe
<mmeeks> noelgrandin: really? =)
* mmeeks interested in that - but about to join a call ;-)
<mmeeks> noelgrandin: surely ~all bitmap use (except perhaps during parallel image loading) is protected by the SolarMutex
<noelgrandin> mmeeks, apparently not in the PDF export code
<noelgrandin> or more accurately PDF convert-to code
<noelgrandin> but if that is the preferred solution I can do that instead
<thorsten> noelgrandin: use cow_wrapper<Foo, ThreadSafeRefCountingPolicy> then?
<noelgrandin> thorsten, that occurred to me, but wasn't sure it wouldn't impact perofmance elswhere, not sure how widely used that thing is
<thorsten> noelgrandin: impossible to tell from the outset ;)
<thorsten> but yeah, rather widely used..
<noelgrandin> thorsten, and in this case, Bitmap holds SalBitmap via std::shared_ptr and BitmapInfoAccess has no real need to hold a Bitmap, it can get all the info it needs directly from the SalBitmap
<thorsten> not sure how often it gets copied/shared though
<quikee> it uses atomic - that should have a minimum inpact?
<noelgrandin> quikee, I could try that, but I figured we're going to bypass Bitmap at some point anyhow, so I'll just get an early start :_
<quikee> noelgrandin: the problem will shift from Bitmap to BitmapEx only
<noelgrandin> quikee, in this case, I only have to fix thread-safe reading from bitmap data
<quikee> but yeah... MapMode sux in general
<noelgrandin> not even sure what all MapMode is used for
<quikee> it is used to cause me headache
<noelgrandin> and given it's rather small size, why it needs to be COW.
<noelgrandin> :-)
<thorsten> noelgrandin: so for vcl stuff, someone at some time surely did some assessment, perhaps even profiling,
<thorsten> noelgrandin: but that might well have been in 1994, so results likely are entirely irrelevant today
<vmiklos> noelgrandin: MapMode is a way to describe how to map from logic units to pixels; given doc model is in logic units (twips or mm100 usually) and the screen is in pixels, it's used ~everywhere
<noelgrandin> thorsten, yeah, just commenting, have no intention of changing that, (other than maybe trying the thread-safe COW thing if my current attempt doesn't work out)
<thorsten> noelgrandin: well if you touch the area anyway, why not un-cow the thing - if even strings were found to benefit from just plain copying ...
<quikee> vmiklos: sure but this mapping shouldn't be done ~everywhere :)
<noelgrandin> hah, and while doing this, found a bug originating from year 2000!
<vmiklos> quikee: agreed
<quikee> My vision for this is to do everything in logical units, converted to the EMU, conversion logic to pixel would only be done in the OutpuDevice. The exception is the UI only, where the conversion would be highly controlled.
<thorsten> quikee: mmh - be aware that Calc especially does a lot of pixel-aligning still
<thorsten> quikee: also Writer is full of trickery, but there at least that's prolly worth killing for good
<noelgrandin> hooo boy, looking deeper, I think I am going to need to just use the SolarMutex hammer on this thing.
<noelgrandin> the various *SalBitmap::AcquireBuffer methods do not look threadsafe at all, even for read-only usage
<SophiaS> Oh, seems related: https://gerrit.libreoffice.org/#/c/55365/
<quikee> thorsten: yes, I know - it is just a vision for now :)
<quikee> noelgrandin: there is nothing thread-safe there
<noelgrandin> quikee, then I'm afraid the SolarMutex it will need to be
<buovjaga> alg: is it temporary that SVGs open in Impress?
<alg> buovjaga: no, by purpose. Idea is to have roundtrip oneday when you save impress as svg now
<alg> buovjaga: was even hard to do :-)
<quikee> noelgrandin: that could have performance implications
<noelgrandin> quikee, alternatives?
<quikee> noelgrandin: none
<buovjaga> alg: ok, you are relieved to hear I did not report a bug :P
Comment 5 Caolán McNamara 2018-06-06 11:24:38 UTC
bug 117984 is presumably related
Comment 6 Julien Nabet 2018-06-06 12:10:18 UTC
Just for info, MapMode should be thread-safe now with https://cgit.freedesktop.org/libreoffice/core/commit/?id=d84517a171a17dfa12f25ad4305a06b20f3b7c76
Comment 7 Michael Meeks 2018-06-06 14:16:37 UTC
Oh - wow ...

#13 0x00007fffef03fb0b in Bitmap::ReleaseAccess (pBitmapAccess=0x7fffb00010b0) at /home/noel/libo/vcl/source/bitmap/bitmap.cxx:389
#14 0x00007fffeecde94a in vcl::ScopedBitmapAccess<BitmapReadAccess, Bitmap, &Bitmap::AcquireReadAccess>::~ScopedBitmapAccess (this=0x7fffbfb8ba50)
    at /home/noel/libo/include/vcl/scopedbitmapaccess.hxx:85
#15 0x00007fffeed64c42 in BitmapEx::GetPixelColor (this=0x7fffb00015e8, nX=116, nY=48) at /home/noel/libo/vcl/source/gdi/bitmapex.cxx:759
#16 0x00007fffe8d1bf92 in drawinglayer::texture::GeoTexSvxBitmapEx::modifyBColor (this=0x7fffb00015e0, rUV=..., rBColor=..., rfOpacity=@0x7fffbfb8bd30: 1)
    at /home/noel/libo/drawinglayer/source/texture/texture3d.cxx:125
#17 0x00007fffe8d1c848 in 

Looks like a disaster not just from a threading perspective - but a performance perspective too =) Creating and destroying this guy:

        Bitmap::ScopedReadAccess pReadAccess( aTestBitmap );

per-pixel can (IIRC) result in a DMA directly to GPU memory and all sorts of horrible slowness (I forget if we cache on the CPU the copy we get back there). Either way - that looks really 'orrible.

I'm rather surprised that we're threading the Bitmap access in the first instance TBH - and then doing manual pixel bashing serially on the CPU while threaded rather than in a shader in unbelievable parallel (hundreds wide) on the GPU - is also a curious choice but ... ;-)

It is far from clear to me that doing tons of atomic references and making 'mapmode thread safe' ;-) [ if that is even the plan ] is the right way to do threading; sounds impossibly granular to me =)

So - what gives ? this looks quite far from ideal to me =)
Comment 8 Michael Meeks 2018-06-06 14:16:38 UTC Comment hidden (obsolete)
Comment 9 Michael Meeks 2018-06-06 14:21:25 UTC
Noel tells me we used to allocate the ScopedReadAccess before we started multithreading - which sounds sensible =) we should keep doing something like that (I guess) in http://cgit.freedesktop.org/libreoffice/core/commit/?id=63e65d1743264dfa26d2aba615d71978e65784e8
Comment 10 Commit Notification 2018-06-06 23:25:42 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b292a27698e85fd9d60c03613c3b0c67835c4dc1

tdf#118029 crashtesting ooo24656-1.doc with --convert-to pdf

It will be available in 6.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.