1130978 - [FFOS7715 v2.1][Search] monkey test crash at libc.so + 0x22038 (VisitEdges fix)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chaochao Huang]

B2G_OS_Version=2.1.0.0-prerelease
Android_Device=scx15_sp7715ea
Android_Manufacturer=Spreadtrum
ProductName=B2G
Android_Board=scx15_sp7715ea
Android_CPU_ABI=armeabi-v7a
Vendor=Mozilla
InstallTime=1422000361
Notes=GL Layers! EGL? EGL+ GL Context? GL Context+ GL Layers+ 
ReleaseChannel=default
Android_CPU_ABI2=armeabi
Version=34.0
Android_Brand=Spreadtrum
ServerURL=https://crash-reports.mozilla.com/submit?id={3c2e2abc-06d4-11e1-ac3b-374f68613e61}&version=34.0&buildid=20150122152411
Android_Hardware=scx15
useragent_locale=en-US
BuildID=20150122152411
ProductID={3c2e2abc-06d4-11e1-ac3b-374f68613e61}
Android_Version=19(REL)
Android_Model=SP7715A
CrashTime=1422031410
StartupTime=1422022678
ProcessType=content
URL=app://search.gaiamobile.org/manifest.webapp

Comment 1

[Chaochao Huang]

0  libc.so + 0x22038
1  libxul.so!VisitNextEdgeBetweenRect [nsRegion.cpp : 394 + 0xd]
2  libxul.so!nsRegion::VisitEdges(void (*)(void*, VisitSide, int, int, int, int), void*) [nsRegion.cpp : 452 + 0x13]
3  libxul.so!mozilla::layers::PadDrawTargetOutFromRegion(mozilla::RefPtr<mozilla::gfx::DrawTarget>, nsIntRegion&) [nsRegion.h : 710 + 0xb]
4  libxul.so!mozilla::layers::ClientTiledLayerBuffer::ValidateTile(mozilla::layers::TileClient, nsIntPoint const&, nsIntRegion const&) [TiledContentClient.cpp : 1251 + 0x3]
5  libxul.so!mozilla::layers::TiledLayerBuffer<mozilla::layers::ClientTiledLayerBuffer, mozilla::layers::TileClient>::Update(nsIntRegion const&, nsIntRegion const&) [TiledLayerBuffer.h : 501 + 0x5]
6  libxul.so!mozilla::layers::ClientTiledLayerBuffer::PaintThebes(nsIntRegion const&, nsIntRegion const&, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*) [TiledContentClient.cpp : 959 + 0x9]
7  libxul.so!mozilla::layers::ClientTiledThebesLayer::RenderHighPrecision(nsIntRegion&, nsIntRegion const&, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*) [ClientTiledThebesLayer.cpp : 246 + 0x5]
8  libxul.so!mozilla::layers::ClientTiledThebesLayer::RenderLayer() [ClientTiledThebesLayer.cpp : 426 + 0xf]
9  libxul.so!SkScalerContext::getFontMetrics(SkPaint::FontMetrics*) [SkScalerContext.cpp : 802 + 0x5]
10  libxul.so!mozilla::layers::ClientContainerLayer::RenderLayer() [ClientContainerLayer.h : 69 + 0x7]
11  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 : 266 + 0x7]
12  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 : 296 + 0xb]
13  libxul.so!nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) [nsDisplayList.cpp : 1349 + 0xf]
14  libxul.so!nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) [nsDisplayList.cpp : 1194 + 0xd]
15  libxul.so!nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) [nsLayoutUtils.cpp : 3026 + 0x7]
16  libxul.so!PresShell::Paint(nsView*, nsRegion const&, unsigned int) [nsPresShell.cpp : 6224 + 0xd]
17  libxul.so!nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) [nsViewManager.cpp : 443 + 0x9]
18  libxul.so!nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) [nsViewManager.cpp : 384 + 0x7]
19  libxul.so!nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [nsRefreshDriver.cpp : 1276 + 0x5]
20  libxul.so!mozilla::RefreshDriverTimer::Tick() [nsRefreshDriver.cpp : 171 + 0xb]
21  libxul.so!nsTimerImpl::Fire() [nsTimerImpl.cpp : 618 + 0x5]
22  libxul.so!nsTimerEvent::Run() [nsTimerImpl.cpp : 711 + 0x3]
23  libxul.so!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp : 823 + 0x5]
24  libxul.so!NS_ProcessNextEvent(nsIThread*, bool) [nsThreadUtils.cpp : 265 + 0xb]
25  libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [MessagePump.cpp : 99 + 0x7]
26  libxul.so!MessageLoop::RunInternal() [message_loop.cc : 234 + 0x5]
27  libxul.so!MessageLoop::Run() [message_loop.cc : 227 + 0x5]
28  libxul.so!nsBaseAppShell::Run() [nsBaseAppShell.cpp : 164 + 0x7]
29  libxul.so!XRE_RunAppShell [nsEmbedFunctions.cpp : 702 + 0x5]
30  libxul.so!MessageLoop::RunInternal() [message_loop.cc : 234 + 0x5]
31  libxul.so!MessageLoop::Run() [message_loop.cc : 227 + 0x5]
32  libxul.so!XRE_InitChildProcess [nsEmbedFunctions.cpp : 539 + 0x5]
33  plugin-container!content_process_main(int, char**) [plugin-container.cpp : 150 + 0x7]
34  libc.so!__libc_init [libc_init_dynamic.cpp : 112 + 0x7]
35  plugin-container + 0x662
36  linker!set_soinfo_pool_protection [linker.cpp : 291 + 0xb]
37  0xbedcb9ab

Comment 2

[yong.ren]

Hi vance:

   please help to assign someone to investigate this crash, thanks

Comment 3

[Steven Yang [:styang]]

ni? Danny and Rex for the follow-up.

Comment 4

[Rex Hung[:rhung]]

Hi, Chaochao,

Please run the following steps to get full SW information for us.
1. git clone https://github.com/Mozilla-TWQA/B2G-flash-tool
2. cd B2G-flash-tool/
3. Connect USB cable to your device and make sure adb works well.
4. Run ./check_version.sh and post all output on the bugzilla.

Comment 5

[yong.ren]

The information you request are as follows:

~/code/B2G-flash-tool$ ./check_versions.sh 
Gaia-Rev        2055fc40a8bd2af1908979cb45da6b7d1c4ced0b
Gecko-Rev       117b02783b5fb93e122ff8bd5791501dbc966f4f
Build-ID        20150122152411
Version         34.0
Device-Name     scx15_sp7715ea
FW-Release      4.4.2
FW-Incremental  93
FW-Date         Thu Jan 22 15:21:20 CST 2015

Comment 6

[Rex Hung[:rhung]]

Same issue is under investigation now, and will post analysis later once it is ready.
Also NI Wenyuan to catch this bug.

Comment 7

[Wenyuan Chi [:wchi]]

Hi Rex,

Yes, this crash looks like bug#1126644.
I am working on it.

Comment 8

[Danny Liang [:dliang]]

Cancel NI due to wchi is working on this.

Comment 9

[Keven Kuo [:kkuo]]

(In reply to wchi from comment #7)
> Hi Rex,
> 
> Yes, this crash looks like bug#1126644.
> I am working on it.

Assign this case to wchi.

--
Keven

Comment 10

[yong.ren]

Hi wchi:

  we sprd colleagues are not authorized to access bug #1126644.  could you please update about this bug's investigate progress, thanks a lot.

Comment 11

[Wenyuan Chi [:wchi]]

Hi yong,

The root cause of bug#1125544 is identified and I am wait for module owner to confirm it.
There is a working fix although it is not reviewed yet.

Would you mind taking the fix and test if it works for this bug#1130978?

Thanks

Comment 12

[Wenyuan Chi [:wchi]]

Attachment: fix.patch

Comment 13

[yong.ren]

(In reply to wchi from comment #12)
> Created attachment 8572987 [details] [diff] [review]
> fix.patch

thanks wchi, I will try this patch.

Comment 15

[Wenyuan Chi [:wchi]]

Attachment: Fix VisitEdges by Jeff

Comment 16

[Wenyuan Chi [:wchi]]

Hi yong,

please try the patch in comment#15. It will be the formal one.
thanks

Comment 17

[Wenyuan Chi [:wchi]]

Hi Jeff,

Would you like to take this bug and bug#1126644 ?
Thanks.

Comment 18

[yong.ren]

(In reply to wchi from comment #16)
> Hi yong,
> 
> please try the patch in comment#15. It will be the formal one.
> thanks

thanks wchi.

Hi chaochao:

  Please help me to try this patch on our 2.1s branch, thanks.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Carrying over Jeff's comment from bug 1126644.

> Comment on attachment 8574028 [details] [diff]
> Fix VisitEdges
> 
> The code is broken because in the else case of VisitNextEdgeBetweenRect we
> assume that r2->x1 < r1->x1. This is not always the case. 
> 
> The fix is to have VisitNextEdgeBetweenRect return whether there's an
> overlap.
> The calling code can than adjust x1 appropriately if r1 != r1_end && r2 !=
> r2_end.
> 
> If you have questions about how this works, ask.
> 
> Ps. I'll remove the 'cout << '

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8574486 [details] [diff] [review]
Fix VisitEdges by Jeff

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

::: gfx/src/nsRegion.cpp
@@ +394,5 @@
>      // we handle the corners by just extending the top and bottom edges
>      visit(closure, side, x1, y, r1->x2+1, y);
>      r1++;
>      x1 = r2->x1 - 1;
> +    return false;

Remove the assignment to x1 here and add a comment saying "we're not sure if the next rect to process will be r1 or r2; the caller function will decide and update x1" (or something to that effect)

::: gfx/tests/gtest/TestRegion.cpp
@@ +543,5 @@
> +    // vertically separated
> +    nsRegion r(nsRect(4, 1, 61, 49));
> +    r.Or(r, nsRect(115, 1, 99, 49));
> +    r.Or(r, nsRect(115, 49, 99, 1));
> +    r.Or(r, nsRect(12, 50, 11, 5));

This test is not easy to follow. Can you make a more simplified one (perhaps in addition to this one)? I think something like this should demonstrate the problem, assuming I understand it correctly:

nsRegion r(nsRect(0, 0, 50, 50));
r.Or(nsRect(100, 0, 50, 50));
r.Or(nsRect(200, 50, 50, 50));

(i.e.:

+-----+     +-----+           
|     |     |     |           
|     |     |     |           
+-----+     +-----+    +-----+
                       |     |
                       |     |
                       +-----+
)

Comment 21

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> Comment on attachment 8574486 [details] [diff] [review]
> Fix VisitEdges by Jeff
> 
> Review of attachment 8574486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/nsRegion.cpp
> @@ +394,5 @@
> >      // we handle the corners by just extending the top and bottom edges
> >      visit(closure, side, x1, y, r1->x2+1, y);
> >      r1++;
> >      x1 = r2->x1 - 1;
> > +    return false;
> 
> Remove the assignment to x1 here and add a comment saying "we're not sure if
> the next rect to process will be r1 or r2; the caller function will decide
> and update x1" (or something to that effect)


We need the assignment so that x1 is properly set when the loop condition in the caller is false.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Hm, ok. I guess the comment should say something like "we assign x1 assuming r2->x1 < r1->x1. However the caller may know better and if so, will update x1 as needed".

Comment 23

[Keven Kuo [:kkuo]]

Due to duplicated case, mark 2.0M as affected.

--
Keven

Comment 24

[Chaochao Huang]

(In reply to yong.ren from comment #18)

Yong ge,

I have applied it to 2.1s branch.

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Uplift? I'm hitting crashes in Fennec 36.0.2 by following STR from bug 1138335 comment 0 on a Nexus 4 - just loading the demo and sitting there a bit will cause crashes. For example:

https://crash-stats.mozilla.com/report/index/07ba7b19-ab6c-4f6d-8fee-62f6e2150317

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8574486 [details] [diff] [review]
Fix VisitEdges by Jeff

Approval Request Comment
[Feature/regressing bug #]: 1023473
[User impact if declined]: Crashes on particular content
[Describe test coverage new/current, TreeHerder]: This code has decent unit test coverage.
[Risks and why]: This code is only used on B2G and Android and it fixes an obvious a pretty obvious crash. It should have no risk on desktop

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

I still see some crashes reported on 39 in the last week and one crash in the last 3 days but that could be clients that haven't yet updated. The final mobile Beta gtb is Monday. I'll check on crash stats again on Monday and make a call about taking the fix in Beta 8.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #28)
> I still see some crashes reported on 39 in the last week and one crash in
> the last 3 days but that could be clients that haven't yet updated.

You could probably check the build ids on the crash reports to see if that's the case. That being said I applied this patch on top of the current mozilla-beta, built it [1], and ran through the STR. 37.0b6 crashes reliably with the STR but the build with this patch did not crash on the same STR. So I think the fix is good and would encourage you to take the uplift.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ce38b5e2141

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8574486 [details] [diff] [review]
Fix VisitEdges by Jeff

Checking build numbers shows them all to be older builds. (Good suggestion kats!) Also, thanks to kats for verifying by applying the patch on Beta. With that verification, I think we're good to take this fix for Beta 8. Beta+ Aurora+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d22e62ca127

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fb9ae74a783a

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/fb9ae74a783a

Comment 34

[Botond Ballo [:botond]]

Bug 1145206 suggests that this may issue may be affecting 36 as well.

Comment 36

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/512388abc72e

Comment 37

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Can you also help to check whether the patch is okay for 2.0M?
Thanks!

Comment 38

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/dabfeaf25fc1

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Too many bugs with very similar titles.