970007 - [tarako]monkey test crash at libxul.so!BufferUnrotate

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[James Zhang (Spreadtrum)]

Attachment: mtlog-now-sp6821a_gonk-99-custom_hudson-jameszhangubtpc-1402081047.tar.bz2

mtlog-now-sp6821a_gonk-99-custom_hudson-jameszhangubtpc-1402081047
-----------------------------------------
[kmsg: dmesg]

[logcat main: logcat.main]

[FFOS minidump: mtlog-now-sp6821a_gonk-99-custom_hudson-jameszhangubtpc-1402081047/dump_parse (the top 10 stack info)]
0  libc.so + 0xdd4c
1  libxul.so!BufferUnrotate(unsigned char*, int, int, int, int, int) [BufferUnrotate.cpp : 35 + 0xb]
2  libxul.so!mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::ThebesLayer*, gfxContentType, unsigned int) [RotatedBuffer.cpp : 548 + 0x7]
3  libxul.so!mozilla::layers::ContentClientBasic::BeginPaintBuffer(mozilla::layers::ThebesLayer*, gfxContentType, unsigned int) + 0xf
4  libxul.so!mozilla::layers::ClientThebesLayer::PaintThebes() [ClientThebesLayer.cpp : 61 + 0x9]
5  libxul.so!mozilla::layers::ClientThebesLayer::RenderLayer() [ClientThebesLayer.cpp : 107 + 0x5]
6  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 81 + 0x5]
7  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 81 + 0x5]
8  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 81 + 0x5]
9  libxul.so!mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) [ClientLayerManager.cpp : 188 + 0x9]

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

already 1.3T?, remove [tarako] in whiteboard

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

hi James, is this easily reproducible and can you share the script for reproducing this?

Comment 3

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #2)
> hi James, is this easily reproducible and can you share the script for
> reproducing this?

We run 10 monkey test and catch this crash every day, from 2.10.
The script is the same as bug 970008.

Comment 4

[Solomon Chiu [:schiu]]

Hi James,
Could you please help to check the dump data or symbol and parse the stack again? Because the information after entering libc.so is all unclear. Typically we should be able to see the call stack, and BufferUnrotate() seems called memcpy() in this point. If there is more information after memcpy() is called, it will be more helpful for this bug.

Comment 5

[James Zhang (Spreadtrum)]

Please see dump_parse in tar, is it helpful?

Operating system: Android
                  0.0.0 Linux 3.0.8+ #1 PREEMPT Sat Feb 8 03:15:47 CST 2014 armv7l Spreadtrum/sp6821a_gonk/sp6821a_gonk:4.0.4.0.4.0.4/OPENMASTER/99:userdebug/test-keys
CPU: arm
     0 CPUs

Crash reason:  SIGSEGV
Crash address: 0x42c910f8

Thread 0 (crashed)
 0  libc.so + 0xdd4c
     r4 = 0x42c91308    r5 = 0x00000210    r6 = 0x00000068    r7 = 0x00000054
     r8 = 0x0000af50    r9 = 0x44e35000   r10 = 0x00000055    fp = 0x000003b8
     sp = 0xbefd27d8    lr = 0x410c7ed3    pc = 0x40104d4c
    Found by: given as instruction pointer in context
 1  libxul.so!BufferUnrotate(unsigned char*, int, int, int, int, int) [BufferUnrotate.cpp : 35 + 0xb]
     sp = 0xbefd27e0    pc = 0x410c7ed3
    Found by: stack scanning
 2  libxul.so!mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::ThebesLayer*, gfxContentType, unsigned int) [RotatedBuffer.cpp : 548 + 0x7]
     r4 = 0x43714c40    r5 = 0xbefd2a60    r6 = 0x43714c4c    r7 = 0x41ac40bd
     r8 = 0x000000ee    r9 = 0x00000042   r10 = 0x00000042    fp = 0x00001000
     sp = 0xbefd2820    pc = 0x410d009b
    Found by: call frame info
 3  libxul.so!mozilla::layers::ContentClientBasic::BeginPaintBuffer(mozilla::layers::ThebesLayer*, gfxContentType, unsigned int) + 0xf
     r4 = 0xbefd2a60    r5 = 0x410d8949    r6 = 0x00001000    r7 = 0xbefd2a60
     r8 = 0xbefd2a94    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2a40    pc = 0x410d8959
    Found by: call frame info
 4  libxul.so!mozilla::layers::ClientThebesLayer::PaintThebes() [ClientThebesLayer.cpp : 61 + 0x9]
     r4 = 0x46540220    r5 = 0x410d8949    r6 = 0x00001000    r7 = 0xbefd2a60
     r8 = 0xbefd2a94    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2a50    pc = 0x410d6b1b
    Found by: call frame info
 5  libxul.so!mozilla::layers::ClientThebesLayer::RenderLayer() [ClientThebesLayer.cpp : 107 + 0x5]
     r4 = 0x46540220    r5 = 0x43714bf0    r6 = 0x00000000    r7 = 0xbefd2cfc
     r8 = 0x453b8228    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2b18    pc = 0x410d6c6b
    Found by: call frame info
 6  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 81 + 0x5]
     r4 = 0xbefd2b38    r5 = 0x424c46fc    r6 = 0x00000000    r7 = 0xbefd2cfc
     r8 = 0x453b8228    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2b30    pc = 0x410d5af5
    Found by: call frame info
 7  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 81 + 0x5]
     r4 = 0xbefd2bd4    r5 = 0x424c46fc    r6 = 0x00000001    r7 = 0xbefd2cfc
     r8 = 0x453b8228    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2bc8    pc = 0x410d5af5
    Found by: call frame info
 8  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 81 + 0x5]
     r4 = 0xbefd2c70    r5 = 0x424c46fc    r6 = 0x00000002    r7 = 0xbefd2cfc
     r8 = 0x453b8228    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2c60    pc = 0x410d5af5
    Found by: call frame info
 9  libxul.so!mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) [ClientLayerManager.cpp : 188 + 0x9]
     r4 = 0x482d1040    r5 = 0x00000000    r6 = 0x410d5821    r7 = 0xbefd2cfc
     r8 = 0x453b8228    r9 = 0x453b8000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2cf8    pc = 0x410d64d1
    Found by: call frame info
10  libxul.so!mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) [ClientLayerManager.cpp : 211 + 0xb]
     r4 = 0x482d1040    r5 = 0x00000002    r6 = 0xbefd2f34    r7 = 0x4171f729
     r8 = 0x44e03e00    r9 = 0x00000000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2d60    pc = 0x410d6811
    Found by: call frame info
11  libxul.so!nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const [nsDisplayList.cpp : 1217 + 0x3]
     r4 = 0x482d1040    r5 = 0x453b8000    r6 = 0xbefd2f34    r7 = 0x4047b940
     r8 = 0x44e03e00    r9 = 0x00000000   r10 = 0x4541d000    fp = 0x424c46fc
     sp = 0xbefd2d78    pc = 0x41742979
    Found by: call frame info
12  libxul.so!nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const [nsDisplayList.cpp : 1074 + 0xd]
     r4 = 0x00000000    r5 = 0xbefd2f34    r6 = 0xbefd32b8    r7 = 0x0000000d
     r8 = 0xbefd3288    r9 = 0xffffffff   r10 = 0xbefd3228    fp = 0x00000004
     sp = 0xbefd2ed8    pc = 0x41742b0d
    Found by: call frame info
13  libxul.so!nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) [nsLayoutUtils.cpp : 2327 + 0x9]
     r4 = 0x46089298    r5 = 0x00000000    r6 = 0x00000304    r7 = 0xbefd2f34
     r8 = 0xbefd3288    r9 = 0xffffffff   r10 = 0xbefd3228    fp = 0x00000004
     sp = 0xbefd2f00    pc = 0x4174b473
    Found by: call frame info
14  libxul.so!PresShell::Paint(nsView*, nsRegion const&, unsigned int) [nsPresShell.cpp : 5855 + 0xd]
     r4 = 0x482d1040    r5 = 0x43405ae0    r6 = 0x00000001    r7 = 0x00000001
     r8 = 0x46089298    r9 = 0xffffffff   r10 = 0x00000000    fp = 0x00000630
     sp = 0xbefd3368    pc = 0x41715891
    Found by: call frame info
15  libxul.so!nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) [nsViewManager.cpp : 421 + 0x19]
     r4 = 0x00000000    r5 = 0x45492100    r6 = 0x454b84f0    r7 = 0xbefd34d0
     r8 = 0x417155a9    r9 = 0x00000001   r10 = 0x43405ae0    fp = 0x00000003
     sp = 0xbefd34d0    pc = 0x414a57f3
    Found by: call frame info
16  libxul.so!nsViewManager::ProcessPendingUpdates() [nsViewManager.cpp : 1053 + 0x9]
     r4 = 0x454b84f0    r5 = 0x454b84f0    r6 = 0xbefd36c0    r7 = 0x404f97b8
     r8 = 0xbefd35e0    r9 = 0xbefd36e4   r10 = 0x00000000    fp = 0x00000003
     sp = 0xbefd3520    pc = 0x414a5875
    Found by: call frame info
17  libxul.so!nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [nsRefreshDriver.cpp : 1208 + 0x5]
     r4 = 0x404f9760    r5 = 0x454b84f0    r6 = 0xbefd36c0    r7 = 0x404f97b8
     r8 = 0xbefd35e0    r9 = 0xbefd36e4   r10 = 0x00000000    fp = 0x00000003
     sp = 0xbefd3528    pc = 0x4171874b
    Found by: call frame info
18  libxul.so!mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) [nsRefreshDriver.cpp : 168 + 0xb]
     r4 = 0x00000001    r5 = 0x000001f7    r6 = 0xa4159c0b    r7 = 0x00000001
     r8 = 0xbefd37df    r9 = 0x4041e90c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3738    pc = 0x417189e3
    Found by: call frame info
19  libxul.so!nsTimerImpl::Fire() [nsTimerImpl.cpp : 561 + 0x5]
     r4 = 0x45421250    r5 = 0x4171898d    r6 = 0x00000002    r7 = 0x00000001
     r8 = 0xbefd37df    r9 = 0x4041e90c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3768    pc = 0x40e6cf2f
    Found by: call frame info
20  libxul.so!nsTimerEvent::Run() [nsTimerImpl.cpp : 645 + 0x5]
     r4 = 0x4041e8e0    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000001
     r8 = 0xbefd37df    r9 = 0x4041e90c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3790    pc = 0x40e6cfdf
    Found by: call frame info
21  libxul.so!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp : 612 + 0x5]
     r4 = 0x4041e8e0    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000001
     r8 = 0xbefd37df    r9 = 0x4041e90c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3798    pc = 0x40e6b64d
    Found by: call frame info
22  libxul.so!NS_ProcessNextEvent(nsIThread*, bool) [nsThreadUtils.cpp : 263 + 0xb]
     r4 = 0x00000000    r5 = 0x404b40c0    r6 = 0x40402d70    r7 = 0x00000001
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd37d8    pc = 0x40e3eb51
    Found by: call frame info
23  libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [MessagePump.cpp : 85 + 0x7]
     r4 = 0x40402d60    r5 = 0x404b40c0    r6 = 0x40402d70    r7 = 0x00000001
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd37e8    pc = 0x40f96b59
    Found by: call frame info
24  libxul.so!MessageLoop::RunInternal() [message_loop.cc : 222 + 0x5]
     r4 = 0x404b40c0    r5 = 0x437044c0    r6 = 0x4041e8e0    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3810    pc = 0x40f8cb01
    Found by: call frame info
25  libxul.so!MessageLoop::Run() [message_loop.cc : 215 + 0x5]
     r4 = 0x404b40c0    r5 = 0x437044c0    r6 = 0x4041e8e0    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3818    pc = 0x40f8cb7f
    Found by: call frame info
26  libxul.so!nsBaseAppShell::Run() [nsBaseAppShell.cpp : 161 + 0x7]
     r4 = 0x00000000    r5 = 0x437044c0    r6 = 0x4041e8e0    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3830    pc = 0x4134d5ad
    Found by: call frame info
27  libxul.so!nsAppStartup::Run() [nsAppStartup.cpp : 276 + 0x5]
     r4 = 0x43703ee0    r5 = 0x40e54885    r6 = 0xbefd3b05    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3840    pc = 0x41978b19
    Found by: call frame info
28  libxul.so!XREMain::XRE_mainRun() [nsAppRunner.cpp : 4008 + 0x5]
     r4 = 0xbefd3a14    r5 = 0x40e54885    r6 = 0xbefd3b05    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0xbefd396c   r10 = 0xbefd3978    fp = 0x00000000
     sp = 0xbefd3848    pc = 0x41951175
    Found by: call frame info
29  libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*) [nsAppRunner.cpp : 4076 + 0x5]
     r4 = 0xbefd3a14    r5 = 0xbefd39ee    r6 = 0x00000000    r7 = 0x00021170
     r8 = 0x40438000    r9 = 0x4043c000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbefd39e8    pc = 0x41953b4b
    Found by: call frame info
30  libxul.so!XRE_main [nsAppRunner.cpp : 4316 + 0x3]
     r4 = 0x00021170    r5 = 0xbefd5bf4    r6 = 0x00000001    r7 = 0x00000000
     r8 = 0xbefd3a14    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbefd3a10    pc = 0x41953cb5
    Found by: call frame info
31  b2g!main [nsBrowserApp.cpp : 163 + 0xf]
     r4 = 0x41953c69    r5 = 0x00000000    r6 = 0x00000001    r7 = 0xbefd5bf4
     r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbefd3b20    pc = 0x000098df
    Found by: call frame info
32  libc.so!__libc_init [libc_init_dynamic.c : 114 + 0x7]
     r4 = 0x00009654    r5 = 0xbefd5bf4    r6 = 0x00000001    r7 = 0xbefd5bfc
     r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbefd5bd8    pc = 0x4010da57
    Found by: call frame info
33  0xb0001dc5
     r4 = 0x00000000    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x00000000
     r8 = 0x00000000    r9 = 0x00000000   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbefd5bf0    pc = 0xb0001dc7
    Found by: call frame info
34  b2g!MOZ_PNG_get_cHRM [pngget.c : 517 + 0x9]
     sp = 0xbefd5c4c    pc = 0x0000b8d7
    Found by: stack scanning
35  b2g + 0x32
     r4 = 0x00000006    r5 = 0x00001000    r6 = 0x00000011    r7 = 0x00000064
     r8 = 0x00000003    sp = 0xbefd5c64    pc = 0x00008034
    Found by: call frame info

Comment 6

[Solomon Chiu [:schiu]]

Hi James,

I did check dump_parse previously, unfortunately it doesn't help much.

Comment 7

[James Zhang (Spreadtrum)]

Talked with ttsai, we'll run hamachi monkey to reproduce this issue, and we'll provide monkey test and hudson script.

Comment 8

[[:fabrice] Fabrice Desré]

James, do you still see this bug?

Comment 9

[James Zhang (Spreadtrum)]

1.We refactory the monkey test script because of some bugs, and delete setting apps before monkey start for avoid usb mass storage issue.
2.We can't meet this crash after refactory script.

Comment 10

[Gregor Wagner [:gwagner]]

Milan, does this belong to your team?

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes.  Some thoughts though:
1. This involves buffer rotation and we're switching away from that in 1.4, so this code will eventually not be hit.
2. This is tagged as 1.3T, and I imagine nobody will want to uplift the tiling from 1.4 to 1.3.
3. Sounds like the crash is gone (comment 9).

Comment 12

[Gregor Wagner [:gwagner]]

Lets reopen if it comes back.

Comment 13

[James Zhang (Spreadtrum)]

Attachment: mtlog-end-sp6821a_gonk-hudson-150-jameszhangubtpc-1403070830.tar.bz2

You can find the crash log in minidump folder.

Comment 14

[James Zhang (Spreadtrum)]

We catch this crash again in 0306 daily build.

Comment 15

[Andreas Gal :gal]

    uint8_t* line = new uint8_t[aByteWidth];

    ...

    for (int y = 0; y < aHeight; y++) {
      int yOffset = y * aByteStride;
-->   memcpy(line, &aBuffer[yOffset + smallStart], smallLen);
      memmove(&aBuffer[yOffset + largeDest], &aBuffer[yOffset + largeStart], largeLen);
      memcpy(&aBuffer[yOffset + smallDest], line, smallLen);
    }

If I read the minidump right we die on first write to line, which we allocated with new[]. Is this maybe an OOM situation? The allocation is not very large (aByteWidth), so thats a bit surprising.

Comment 16

[Benoit Girard (:BenWa)]

new[] should be infaliable

Comment 17

[Andreas Gal :gal]

Do you have another explanation for this?

Comment 18

[Steven Yang [:styang]]

Hi Alan, could you check the new dump? thanks

Comment 19

[bhavana bajaj [:bajaj]]

:Benwa, is this something you can help investigate ?

Comment 20

[Benoit Girard (:BenWa)]

The crash address comes from r4 + some offset. r4 is probably our buffer. My suggestion is to check which pointer is at r4 (line or aBuffer) which will let us know if we need to chase down new[] weirdness or that perhaps aBuffer doesn't really have the bounds that it complains to and chase down why.

James can you still reproduce this and check which buffer and why that buffer wouldn't wouldn't be large enough?

Comment 21

[James Zhang (Spreadtrum)]

(In reply to Benoit Girard (:BenWa) from comment #20)
> The crash address comes from r4 + some offset. r4 is probably our buffer. My
> suggestion is to check which pointer is at r4 (line or aBuffer) which will
> let us know if we need to chase down new[] weirdness or that perhaps aBuffer
> doesn't really have the bounds that it complains to and chase down why.
> 
> James can you still reproduce this and check which buffer and why that
> buffer wouldn't wouldn't be large enough?

Which buffer need we check?
Ying, please track this display buffer crash bug.

Comment 22

[James Zhang (Spreadtrum)]

We need minidump screen shot patch in Bug 970008. 
https://bugzilla.mozilla.org/attachment.cgi?id=8388039

Comment 23

[ying.xu]

(In reply to Benoit Girard (:BenWa) from comment #20)
> James can you still reproduce this and check which buffer and why that
> buffer wouldn't wouldn't be large enough?

We get this crash with monkey test.
It's not easy to capture the crash scene and debug it.
So, I will add some logs and run monkey test.

Comment 24

[Joe Cheng [:jcheng] (please needinfo)]

triage: minus as we have no solid information for next action, please renominate when there are more information. thanks

Comment 26

[Solomon Chiu [:schiu]]

The steps in Bug#976656 provided by Peipei seems can used to reproduce this bug more easily.

Method-1:
1. Launch Music
2. Switch to Song list
3. Swipe up and down and select one song to play.
4. Repeat 2~3 for several times, you will see Music crash.

Method-2:
1. Prepare a long list of messages in SMS(or a lot of music in Music Song list)
2. Launch SMS
3. Swipe up and down to browse messages

Method-3
1. Go to Settings->Screen lock
2. Enable passcode lock and create a new passcode
    --> Crash may happen here
3. If no crash at step 2, then go back to Screen lock
4. Try to disable passcode lock
   --> Crash may happen here

P.S. Before reproducing, make sure APZ is turned off.

Comment 27

[Solomon Chiu [:schiu]]

What I found today are:

1. The crash typically occures in memmove(), which called by http://dxr.mozilla.org/mozilla-central/source/gfx/layers/BufferUnrotate.cpp?from=bufferunrotate.cpp#59
2. In that time, BuffferUnrotate received some wired parameters, where aByteWidth is large than aByteStride. For example, aByteWidth = 1280, aByteStride = 640. Which means aByteWidth, aByteStride, format are inconsistent when RotatedBuffer:BeginPaint() calls BufferUnrotate().

Comment 28

[Natalya Kot [:nkot]]

(In reply to Solomon Chiu [:schiu] from comment #26)
> The steps in Bug#976656 provided by Peipei seems can used to reproduce this
> bug more easily.

> Method-3
> 1. Go to Settings->Screen lock
> 2. Enable passcode lock and create a new passcode
>     --> Crash may happen here
> 3. If no crash at step 2, then go back to Screen lock
> 4. Try to disable passcode lock
>    --> Crash may happen here

Can consistently reproduce this STR too.

Comment 29

[James Zhang (Spreadtrum)]

spreadtrum major bug.

Comment 30

[Joe Cheng [:jcheng] (please needinfo)]

1.3T+ partner major stabiltiy bug

Comment 31

[Jerry Shih[:jerry] (UTC+8) (inactive)]

in HelpersCario.h:

static inline cairo_content_t
GfxFormatToCairoContent(SurfaceFormat format)
{
  switch (format)
  {
    ....
    case SurfaceFormat::B8G8R8X8:
    case SurfaceFormat::R5G6B5:  //fall through
      return CAIRO_CONTENT_COLOR;
    ....
  }
}

static inline SurfaceFormat
CairoContentToGfxFormat(cairo_content_t content)
{
  switch (content)
  {
    ....
    case CAIRO_CONTENT_COLOR:
      // BEWARE! format may be 565
      return SurfaceFormat::B8G8R8X8;
    ....
  }

  return SurfaceFormat::B8G8R8A8;
}

----
Maybe we have this problem:
SurfaceFormat::R5G6B5 => CAIRO_CONTENT_COLOR => SurfaceFormat::B8G8R8X8

Comment 33

[Peter Chang[:pchang]]

Attachment: WIP

From [1] and comment 31, it is possible to get incorrect colorformat when cairocontent is color type[2].

In tarako, the framebuffer format is rgb565.
Therefore, the colorformat of gfxASurface with GFX_CONTENT_COLOR is rgb565.

If we are trying to create a DrawTargetCairo with this gfxASurface, DrawTargetCairo will get incorrect color format(BRGX8888) that mentioned above .

And it caused this crash and rendering artifact(bug 990386).

Attached the WIP patch to pass correct colorformat based on platform configuration to DrawTargetCairo.

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#1161
[2]http://dxr.mozilla.org/mozilla-central/source/gfx/2d/HelpersCairo.h#207

Comment 34

[Peter Chang[:pchang]]

Based on info from bug 990386 comment 16, I'm using 1.3 leo to verify the WIP.
Laeter I will check this WIP works on tarako or not.

Comment 35

[Vincent Liu[:vliu]]

(In reply to peter chang[:pchang][:peter] from comment #34)
> Based on info from bug 990386 comment 16, I'm using 1.3 leo to verify the
> WIP.
> Laeter I will check this WIP works on tarako or not.

With the patch in Comment 33, the crash never happens on my tarako device following the Method-1 in Comment 26. But even so, I still suggest that verifying it with monkey test.

Comment 36

[Peter Chang[:pchang]]

Comment on attachment 8404888 [details] [diff] [review]
WIP

Review of attachment 8404888 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ +649,4 @@
>  RefPtr<DrawTarget>
>  gfxPlatform::CreateDrawTargetForSurface(gfxASurface *aSurface, const IntSize& aSize)
>  {
> +  SurfaceFormat format = Optimal2DFormatForContent(aSurface->GetContentType());

Here based on platform configuration passes the SurfaceFormat of gfxASurface to DrawTargetCairo to avoid the incorrect colorformat of content color type.

Since this is 1.3T blocker, I prefer not to do big modification. Therefore, this patch only adds new APIs with one new SurfaceFormat parameter, compared with original API.

Comment 37

[u459114]

Comment on attachment 8404888 [details] [diff] [review]
WIP

Review of attachment 8404888 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +1173,5 @@
> +  }
> +
> +  return true;
> +}
> +

bool
DrawTargetCairo::InitAlreadyReferenced(cairo_surface_t* aSurface, const IntSize& aSize)
{
  SurfaceFormat format = CairoContentToGfxFormat(cairo_surface_get_content(aSurface));
  return InitAlreadyReferenced(aSuface, aSize, format);
}

::: gfx/2d/Factory.cpp
@@ +667,3 @@
>  TemporaryRef<SourceSurface>
>  Factory::CreateSourceSurfaceForCairoSurface(cairo_surface_t* aSurface,
>                                              SurfaceFormat aFormat)

I think you can remove Factory::CreateDrawTargetForCairoSurface(cairo_surface_t* aSurface, const IntSize& aSize)
since the only caller of this function is in gfxPlatform.cpp and you remove that call in this patch as well

Comment 38

[u459114]

There is no correct map between SurfaceFormat::R5G6B5 and _cairo_content

typedef enum _cairo_content {  
  CAIRO_CONTENT_COLOR   = 0x1000,
  CAIRO_CONTENT_ALPHA   = 0x2000,
  CAIRO_CONTENT_COLOR_ALPHA = 0x3000
} cairo_content_t;

We can dig into cario add a new type in _cairo_content, or just like what Peter did, covert that format mapping in DrawTargetCairo wapper layer.

Comment 39

[Peter Chang[:pchang]]

Comment on attachment 8404888 [details] [diff] [review]
WIP

Review of attachment 8404888 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +1173,5 @@
> +  }
> +
> +  return true;
> +}
> +

Calling CairoContextToGfxFormat with cairo_content_color type will cause incorrect colorformat[1] when the surface is created as rgb565 in the beginning.

That's why I tried to pass SurfaceFormat from gfxPlatform::CreateDrawTargetForSurface that contains the correct color format to DrawTargetCairo.

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/2d/HelpersCairo.h?from=CairoContentToGfxFormat&case=true#209

::: gfx/2d/Factory.cpp
@@ +667,3 @@
>  TemporaryRef<SourceSurface>
>  Factory::CreateSourceSurfaceForCairoSurface(cairo_surface_t* aSurface,
>                                              SurfaceFormat aFormat)

There are several places to call Factory::CreateDrawTargetForCairoSurface.

http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp#75
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/TextureClientX11.cpp#138

Comment 40

[u459114]

(In reply to peter chang[:pchang][:peter] from comment #39)
> ::: gfx/2d/Factory.cpp
> @@ +667,3 @@
> >  TemporaryRef<SourceSurface>
> >  Factory::CreateSourceSurfaceForCairoSurface(cairo_surface_t* aSurface,
> >                                              SurfaceFormat aFormat)
> 
> There are several places to call Factory::CreateDrawTargetForCairoSurface.
> 
> http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp#75
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/
> TextureClientX11.cpp#138
dt = Factory::CreateDrawTargetForCairoSurface(image->CairoSurface(), size); <-- size, not format
Factory::CreateDrawTargetForCairoSurface(mSurface->CairoSurface(), size); <-- size, not format

Comment 41

[u459114]

(In reply to peter chang[:pchang][:peter] from comment #39)
> Comment on attachment 8404888 [details] [diff] [review]
> WIP
> 
> Review of attachment 8404888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCairo.cpp
> @@ +1173,5 @@
> > +  }
> > +
> > +  return true;
> > +}
> > +
> 
> Calling CairoContextToGfxFormat with cairo_content_color type will cause
> incorrect colorformat[1] when the surface is created as rgb565 in the
> beginning.
> 
> That's why I tried to pass SurfaceFormat from
> gfxPlatform::CreateDrawTargetForSurface that contains the correct color
> format to DrawTargetCairo.
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/gfx/2d/HelpersCairo.
> h?from=CairoContentToGfxFormat&case=true#209
OK, here is what exactly I mean. 
bool
DrawTargetCairo::InitAlreadyReferenced(cairo_surface_t* aSurface, const IntSize& aSize, SurfaceFormat aFormat)
{
  mContext = cairo_create(aSurface);
  mSurface = aSurface;
  mSize = aSize;
  mFormat = aFormat;
		 
  if (mFormat == FORMAT_B8G8R8A8 ||
      mFormat == FORMAT_R8G8B8A8) {
    SetPermitSubpixelAA(false);
  } else {
    SetPermitSubpixelAA(true);
  }
		 
  return true;
}

// Original one, you can make it simpler since you create a new one
bool
DrawTargetCairo::InitAlreadyReferenced(cairo_surface_t* aSurface, const IntSize& aSize)
{
  SurfaceFormat format = CairoContentToGfxFormat(cairo_surface_get_content(aSurface));
  return InitAlreadyReferenced(aSuface, aSize, format);
}

Comment 42

[u459114]

(In reply to C.J. Ku[:CJKu] from comment #40)
> (In reply to peter chang[:pchang][:peter] from comment #39)
> > ::: gfx/2d/Factory.cpp
> > @@ +667,3 @@
> > >  TemporaryRef<SourceSurface>
> > >  Factory::CreateSourceSurfaceForCairoSurface(cairo_surface_t* aSurface,
> > >                                              SurfaceFormat aFormat)
> > 
> > There are several places to call Factory::CreateDrawTargetForCairoSurface.
> > 
> > http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp#75
> > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/
> > TextureClientX11.cpp#138
> dt = Factory::CreateDrawTargetForCairoSurface(image->CairoSurface(), size);
> <-- size, not format
> Factory::CreateDrawTargetForCairoSurface(mSurface->CairoSurface(), size);
> <-- size, not format
Oh, I am sorry, yes, this function is been called here
http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibNativeRenderer.cpp#626

Comment 43

[Peter Chang[:pchang]]

(In reply to C.J. Ku[:CJKu] from comment #41)
> // Original one, you can make it simpler since you create a new one
> bool
> DrawTargetCairo::InitAlreadyReferenced(cairo_surface_t* aSurface, const
> IntSize& aSize)
> {
>   SurfaceFormat format =
> CairoContentToGfxFormat(cairo_surface_get_content(aSurface));
>   return InitAlreadyReferenced(aSuface, aSize, format);
> }
I see, it's simpler.

Actually I had another WIP is to add the SurfaceFormat* in gfxPlatform::CreateDrawTargetForSurface and the default value is nullptr. Therefore, it doesn't need to add new API calls and the modification should be small.

Comment 44

[Peter Chang[:pchang]]

Attachment: WIP v2

Comment 45

[u459114]

Comment on attachment 8405420 [details] [diff] [review]
WIP v2

Review of attachment 8405420 [details] [diff] [review]:
-----------------------------------------------------------------

Even better, thanks

Comment 46

[u459114]

Comment on attachment 8405420 [details] [diff] [review]
WIP v2

Review of attachment 8405420 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +1152,1 @@
>  

nitty
mFormat = aFormat ? *aFormat : CairoContentToGfxFormat(cairo_surface_get_content(aSurface));

Comment 47

[Peter Chang[:pchang]]

(In reply to C.J. Ku[:CJKu] from comment #46)
> Comment on attachment 8405420 [details] [diff] [review]
> WIP v2
> 
> Review of attachment 8405420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCairo.cpp
> @@ +1152,1 @@
> >  
> 
> nitty
> mFormat = aFormat ? *aFormat :
> CairoContentToGfxFormat(cairo_surface_get_content(aSurface));

Will change in next patch

Comment 48

[Peter Chang[:pchang]]

Attachment: WIP v3

Based on comment 33, attach WIP v3 to avoid incorrect colorformat when using CairoSurface.

Comment 49

[Benoit Girard (:BenWa)]

Comment on attachment 8405951 [details] [diff] [review]
WIP v3

Review of attachment 8405951 [details] [diff] [review]:
-----------------------------------------------------------------

moz2d changes -> Bas

Comment 51

[Andreas Gal :gal]

Please land the patch with r=gal and get review from Bas after the fact. If he has change requests we can do a follow up. This is blocking the next round of testing.

Comment 52

[Peter Chang[:pchang]]

Attachment: patch (carry r+)

Based on comment 51, upload patch with r+.

Comment 53

[Peter Chang[:pchang]]

(In reply to Benoit Girard (:BenWa) from comment #49)
> Comment on attachment 8405951 [details] [diff] [review]
> WIP v3
> 
> Review of attachment 8405951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> moz2d changes -> Bas

I will create another bug to discuss the 2D API chanages.

Comment 54

[Peter Chang[:pchang]]

Please help to land attachment 8406607 [details] [diff] [review] to 1.3T code base.

Comment 55

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/integration/b2g-inbound/rev/73f991528355

Comment 56

[Andreas Gal :gal]

I checked in with Bas:

"I had a look, this makes sense and looks fine to me.

Bas"

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a62a1337c765

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73f991528355

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a62a1337c765

Comment 60

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/9574c5fce22e