989408 - crash in mozilla::layers::PLayerTransactionChild::SendPLayerConstructor(mozilla::layers::PLayerChild*)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Brogan Zumwalt [Inactive]]

This bug was filed from the Socorro interface and is 
report bp-531e51f8-71a0-4eea-b068-802232140328.
=============================================================
Description:
In FTU opening and closing Firefox Privacy policy page rapidly results in crash to homescreen

Repro Steps:
1) Updated Buri to BuildID: 20140328000202
2) Sign in to wireless network in FTU
3) Select next in FTU until reaching Geolocation page
4) Press "More about your privacy link"
5) Click Firefox OS
6) Close page by pressing "X"
7) Repeat steps 5 & 6 rapidly and repeatedly

Actual:
Device crashes to homescreen.

Expected:
User is taken back and forth between two pages.

Environmental Variables:
Device: Buri v1.4 Mozilla RIL
BuildID: 20140328000202
Gaia: e07a5915737727ba22c06f270ee7af6572f746f3
Gecko: 33e7fd745c1b
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Notes:
Repro frequency: 3/3, 100%
See attached: logcat

crash in mozilla::layers::PLayerTransactionChild::SendPLayerConstructor(mozilla::layers::PLayerChild*)

Comment 1

[Brogan Zumwalt [Inactive]]

Attachment: Logcat

Attached logcat. Unable to include logcat while Console and Gaia Debug Traces are enabled due to lack of access to dev options inside FTU.

Comment 2

[Jason Smith [:jsmith]]

I'm pretty sure this isn't reproducing on 1.3, but let's confirm that to be sure.

Comment 3

[Josh Schmitt [Joshs]]

(In reply to Jason Smith [:jsmith] from comment #2)
> I'm pretty sure this isn't reproducing on 1.3, but let's confirm that to be
> sure.

I was able to reproduce this issue on the 1.3 build, the device crashed to homescreen.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140328004002
Gaia: 523196662f4d19c7898d5f4773da020c39cfe57e
Gecko: aa1d48ab3715
Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 4

[Jason Smith [:jsmith]]

Can you check if this happens on 1.1?

Comment 5

[Brogan Zumwalt [Inactive]]

Does NOT occur in 1.1.

Menu navigation also appears to be different. Instead of the "X" in the top corner on the Firefox OS privacy policy page there is a back button on the bottom of the screen.

Environmental Variables:
Device: Buri v1.1 Mozilla RIL
BuildID: 20140331041208
Gaia: 44a2ddf63373f8e95c784faf4ed4d60081699c61
Gecko: 170d87349060
Version: 18.0
Firmware Version: v1.2-device.cfg

Comment 6

[Jason Smith [:jsmith]]

Can this be reproduced this in a more realistic user flow in 1.3 (i.e. not rapid interactions, but normal user workflows of operation).

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Add log around TabParent and TabChild

Add some logouts and KillHard() call in ContentParent::ProcessingError(Result what) is commented out for ease of debugging.

Comment 8

[Sotaro Ikeda [:sotaro]]

On recent v1.4 hamachi, I could reproduce the crash but the crash happened around IndexedDB's IPC. Anyway, from the source code, ShadowLayerForwarder's code also has a problem. Need to fix it.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: logcat of the crash with manually added log

From the log, content's initialization happens after TabChild::ActorDestroy() and TabParent::Destroy() is called.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> On recent v1.4 hamachi, I could reproduce the crash but the crash happened
> around IndexedDB's IPC. Anyway, from the source code, ShadowLayerForwarder's
> code also has a problem. Need to fix it.

I always face an application's crash because of IndexedDB error. I did not see PLayerTransactionChild::SendPLayerConstructor().

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add checking IPCOpen to ShadowLayerForwarder

Comment 12

[Sotaro Ikeda [:sotaro]]

Set to 1.4? flag. This bug blocks 1.4+ bug.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8406508 [details] [diff] [review]
patch - Add checking IPCOpen to ShadowLayerForwarder

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

Thanks! One comment below:

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +585,5 @@
>  bool
>  ShadowLayerForwarder::IsSameProcess() const
>  {
> +  if (!mShadowManager->IPCOpen()) {
> +    return false;

Could this instead be a MOZ_ASSERT(mShadowManager->IPCOpen()), and fix the callers to check IPCOpen() before calling IsSameProcess()?

That would make more sense to me, than encoding "IPC link down" as "not same process" which seems ad-hoc.

There should be very few callers to fix, as I only very recently added this IsSameProcess thing for MemoryTexture's.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Note that this comment is only about this one instance, in IsSameProcess. All other places look fine.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> Set to 1.4? flag. This bug blocks 1.4+ bug.

Blocks a blocker.

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 8406508 [details] [diff] [review]
patch - Add checking IPCOpen to ShadowLayerForwarder

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

About bjacob's comment: we almost never use debug builds for automated testing on b2g so they are not as useful as they are on desktop. I would rather have at least the "if -> return" and optionally have an assert on top of that for debug builds.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Benoit Jacob [:bjacob] from comment #13)
> Comment on attachment 8406508 [details] [diff] [review]
> patch - Add checking IPCOpen to ShadowLayerForwarder
> 
> Review of attachment 8406508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! One comment below:
> 
> ::: gfx/layers/ipc/ShadowLayers.cpp
> @@ +585,5 @@
> >  bool
> >  ShadowLayerForwarder::IsSameProcess() const
> >  {
> > +  if (!mShadowManager->IPCOpen()) {
> > +    return false;
> 
> Could this instead be a MOZ_ASSERT(mShadowManager->IPCOpen()), and fix the
> callers to check IPCOpen() before calling IsSameProcess()?

I do not want to expose IPC internal status out side of ShadowLayerForwarder. I do not even like current patch. I create it just for band-aid fix.

Comment 18

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=c4157576588d

Comment 19

[Sotaro Ikeda [:sotaro]]

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/987db9c9b103