933483 - [Poppit] App freezes when changing Poppit skill level

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Elisa Helton]

Attachment: Poppit Skill Level Screen frozen.png

Summary: [Poppit] App freezes when changing Poppit skill level

Repro Steps:
-Open Poppit App
-Tap screen to begin game
-Press Menu button
-Select “Options” from Main Menu
-Press any level button (Easy, Medium, or Hard)

Actual: A page will come up that says “Changing skill level will erase current progress. Do you want to change the skill level?” There are buttons for ‘Cancel’ and ‘OK’ but pressing them does not produce any behavior. Also impossible to go back to previous page, so the user has to kill the app entirely. 
 
Expected: Pressing ‘Cancel’ would return the user to the previous Options page. Pressing ‘OK’ would change the skill level appropriately.

Environmental Variables
Build ID: 20131028225522
Gecko:
Gaia:
Platform Version: 26.0a2

Notes: Diagnosed on Keon device FFOS v1.2 nightly build

Comment 1

[Jason Smith [:jsmith]]

This is a Gaia System bug - might be a dupe of an existing bug as well. Let me look around.

Comment 2

[Jason Smith [:jsmith]]

Pretty sure this is a regression, but someone should confirm if this reproduces on 1.1 or not.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

The only thing I could figure out is it's using window.confirm and something is wrong with that.

Comment 4

[Sarah Parsons]

I was not able to reproduce this issue on Buri 1.1 Build ID: 20131101041316

Gaia   39b0203fa9809052c8c4d4332fef03bbaf0426fc
SourceStamp 31fa87bfba88
BuildID 20131101041316
Version 18.0

This issue does however reproduce on Buri 1.2 Build ID:20131101004000

Gaia   e717aec947571f5daf923c040a82f9f0719bb526
SourceStamp 54de309e18a9
BuildID 20131101004000
Version 26.0

Comment 5

[Jason Smith [:jsmith]]

Just realized - we can't get a regression window here easily because Poppit was OOMing until quite recently when a SkiaGL fix came in.

If we were to get a regression window here, we would need to get a reduced test case extracted out of Poppit that would not trigger the SkiaGL bug. Don't know how easy that's going to be.

I'll let a developer look into first to indicate if we need dig in deeper here or not.

Comment 6

[Preeti Raghunath(:Preeti)]

Alive,

Please take a look.

Comment 7

[bhavana bajaj [:bajaj]]

Alive, what are the next steps here or do you need help with investigation or reproduction here ? Would this be resolved in the upcoming sprint deadline of (11/22)?

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Sorry for late reply.
It's using window.confirm but that one is working in other apps, so this is not a general case for window.confirm.

I think we need gecko guy to look at the hanging since I don't think gaia has anything to do with "hanging".

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I debug a little bit, and noticed that the app is not really hanging. We receive *only* 45 confirm prompts so if you click 45 times on the ok button then it works.

Since it works for some other apps, I wonder if we can have access to the poppip underlying code for this screen? I'm curious to see if this is a bug on our implement of mozbrowser of if it is a bug in the application code directly.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> I debug a little bit, and noticed that the app is not really hanging. We
> receive *only* 45 confirm prompts so if you click 45 times on the ok button
> then it works.
> 
> Since it works for some other apps, I wonder if we can have access to the
> poppip underlying code for this screen? I'm curious to see if this is a bug
> on our implement of mozbrowser of if it is a bug in the application code
> directly.

Underlying code is here - https://marketplace.firefox.com/downloads/file/218874/poppit-1.0.5.zip.

I'm doubtful this is a bug in the app's code - we aren't seeing this reproduce on 1.1. This only reproduces on 1.2+.

Comment 11

[Jason Smith [:jsmith]]

Attachment: Poppit App

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> I debug a little bit, and noticed that the app is not really hanging. We
> receive *only* 45 confirm prompts so if you click 45 times on the ok button
> then it works.
> 
> Since it works for some other apps, I wonder if we can have access to the
> poppip underlying code for this screen? I'm curious to see if this is a bug
> on our implement of mozbrowser of if it is a bug in the application code
> directly.

Vivien since you seem to debug this a bit, would you mind owning this bug and helping with investigation further ?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> 
> Vivien since you seem to debug this a bit, would you mind owning this bug
> and helping with investigation further ?

Well I didn't have any real idea about what's going on here. I just looked to see how many requests we received from the content. 

(In reply to Jason Smith [:jsmith] from comment #10)
> Underlying code is here -
> https://marketplace.firefox.com/downloads/file/218874/poppit-1.0.5.zip.
>
Thanks.
 
> I'm doubtful this is a bug in the app's code - we aren't seeing this
> reproduce on 1.1. This only reproduces on 1.2+.

That makes sense. The code also seems to just do a simple confirm. Not sure what's going on here.

Comment 14

[Preeti Raghunath(:Preeti)]

Andrew,

Can you please check and reassign appropriately?

Comment 15

[(no longer active)]

Hmm we sort of have this problem on desktop too if you manage to create multiple alerts/confirms...  I think dolske knows a bit about that code at least on desktop.

Comment 16

[(no longer active)]

Also, if the same app code doesn't show this behavior on 1.1, then this is probably a bug in our code and somebody should bisect it.

Comment 17

[Jason Smith [:jsmith]]

The regression range here doesn't have the right changesets below:

Gaia   39b0203fa9809052c8c4d4332fef03bbaf0426fc
SourceStamp 31fa87bfba88
BuildID 20131101041316
Version 18.0

Sarah - Can you provide the right changesets here?

Comment 18

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #17)
> The regression range here doesn't have the right changesets below:
> 
> Gaia   39b0203fa9809052c8c4d4332fef03bbaf0426fc
> SourceStamp 31fa87bfba88
> BuildID 20131101041316
> Version 18.0
> 
> Sarah - Can you provide the right changesets here?

Disregard - this is the right information.

Comment 19

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> Also, if the same app code doesn't show this behavior on 1.1, then this is
> probably a bug in our code and somebody should bisect it.

Unless we have a reduced test case to reproduce the bug, we can't bisect this. Poppit was OOMing for most of the 1.2 release, so it will prevent being able to test this bug for a regression range.

Comment 20

[(no longer active)]

(In reply to comment #19)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> > Also, if the same app code doesn't show this behavior on 1.1, then this is
> > probably a bug in our code and somebody should bisect it.
> 
> Unless we have a reduced test case to reproduce the bug, we can't bisect this.
> Poppit was OOMing for most of the 1.2 release, so it will prevent being able to
> test this bug for a regression range.

Hmm, that's unfortunate.  Can we bisect on a device with better specs such as a Nexus 4?

Comment 21

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> (In reply to comment #19)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> > > Also, if the same app code doesn't show this behavior on 1.1, then this is
> > > probably a bug in our code and somebody should bisect it.
> > 
> > Unless we have a reduced test case to reproduce the bug, we can't bisect this.
> > Poppit was OOMing for most of the 1.2 release, so it will prevent being able to
> > test this bug for a regression range.
> 
> Hmm, that's unfortunate.  Can we bisect on a device with better specs such
> as a Nexus 4?

Hmm...we might be able to do that with a Leo device, as the Leo device has more memory.

Can someone try getting a regression range here using a Leo device?

Comment 22

[Justin Dolske [:Dolske]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> Hmm we sort of have this problem on desktop too if you manage to create
> multiple alerts/confirms...  I think dolske knows a bit about that code at
> least on desktop.

I'd be curious as to what the call-stack (JS+native) is when the 2nd prompt is triggered. I was sorta thinking Gecko was blocking the recursive calls, but I'm not sure we actually do (since it works but is just horrible UI if a page manages to trigger it).

Comment 9 also kind implies this is a bug in the app (for calling confirm() multiple times, which is silly), although it would be nice if we just outright blocked that.

Comment 23

[(no longer active)]

(In reply to comment #22)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > Hmm we sort of have this problem on desktop too if you manage to create
> > multiple alerts/confirms...  I think dolske knows a bit about that code at
> > least on desktop.
> 
> I'd be curious as to what the call-stack (JS+native) is when the 2nd prompt is
> triggered. I was sorta thinking Gecko was blocking the recursive calls, but I'm
> not sure we actually do (since it works but is just horrible UI if a page
> manages to trigger it).

Well, we protect against the simple cases (such as timeouts and intervals), but not against everything.  See this test case for example <http://people.mozilla.org/~eakhgari/confirm-dance.html> which uses XHR events dispatched to the confirm's nested event loop.

> Comment 9 also kind implies this is a bug in the app (for calling confirm()
> multiple times, which is silly), although it would be nice if we just outright
> blocked that.

Yeah.  I'm really curious to know how this app manages to do this regardless.

Comment 24

[Sarah Parsons]

This issue started to occur on the Leo 1.2 Build ID: 20130926004001

Gaia   1e9470b9b6df630eddf1c4c8b25b3170ee786b0e
SourceStamp caa6b17647c5
BuildID 20130926004001
Version 26.0a2

Last working Leo 1.2 Build ID: 20130925004005

Gaia   b0e4a1333bb7bf0a749a384ba99e4f03f111e39a
SourceStamp fb764e648a8f
BuildID 20130925004005
Version 26.0a2

Comment 25

[Jason Smith [:jsmith]]

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=fb764e648a8f&tochange=caa6b17647c5

Comment 26

[Jason Smith [:jsmith]]

Can you also get a logcat?

Comment 27

[Jason Smith [:jsmith]]

So this definitely doesn't look Gaia related - there's nothing that landed in the system app codebase on 9/25/2013 on the 1.2 branch.

Comment 28

[Sarah Parsons]

Attachment: Poppit_933483_logcat

Comment 29

[Sarah Parsons]

See logcat attached.

Comment 30

[Jason Smith [:jsmith]]

Hmm...the logcat I got included the following:

11-19 14:19:13.629: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 387}]

Comment 31

[Jason Smith [:jsmith]]

11-19 14:21:43.869: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "app://{e8f991fc-a345-4d30-a48e-d5f3e34c668c}/lib/impact/input.js" line: 186}]
11-19 14:21:43.939: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "app://{e8f991fc-a345-4d30-a48e-d5f3e34c668c}/lib/impact/input.js" line: 211}]
11-19 14:21:43.949: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "app://{e8f991fc-a345-4d30-a48e-d5f3e34c668c}/lib/impact/input.js" line: 163}]
11-19 14:21:43.949: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "app://{e8f991fc-a345-4d30-a48e-d5f3e34c668c}/lib/impact/input.js" line: 186}]
11-19 14:21:43.949: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "app://{e8f991fc-a345-4d30-a48e-d5f3e34c668c}/lib/impact/input.js" line: 211}]
11-19 14:21:43.949: E/GeckoConsole(538): [JavaScript Error: "too much recursion" {file: "app://{e8f991fc-a345-4d30-a48e-d5f3e34c668c}/lib/impact/input.js" line: 163}]

Comment 32

[Jason Smith [:jsmith]]

Interestingly enough, if you click the prompt enough, it will eventually disappear, but touch events will stop working entirely in the app at that point.

Comment 33

[Jason Smith [:jsmith]]

bug 914776 looks like a suspicious candidate for causing this.

mwu - what do you think?

Comment 34

[Michael Wu [:mwu]]

Very unlikely.

Comment 35

[Gregor Wagner [:gwagner]]

In BrowserElementChildPreload we call _waitForResult and this seem to cause too much recursion:


I/Gecko:ProcessPriorityManager(  179): [Poppit!™, child-id=3, pid=922] Got wake lock changed event. Now mHoldsCPUWakeLock=1, mHoldsHighPriorityWakeLock=0
I/Gecko   (  922): [Child 922] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../../content/html/content/src/HTMLAudioElement.cpp, line 235
I/Gecko   (  922): BrowserElementChildPreload - _waitForResult([object XrayWrapper [object Window @ 0xb279e940 (native @ 0xb33e6ad0)]])
I/Gecko   (  922): BrowserElementChildPreload - Entering modal state (outerWindowID=1, innerWindowID=3)
I/Gecko   (  922): BrowserElementChildPreload - Nested event loop - begin
W/AudioFlinger(  173): Thread AudioOut_2 cannot connect to the power manager service
E/FastMixer(  173): did not receive expected priority boost
D/audio_hw_primary(  173): start_output_stream: enter: usecase(1: low-latency-playback) devices(0x2)
D/audio_hw_primary(  173): select_devices: out_snd_device(2: speaker) in_snd_device(0: none)
D/audio_hw_primary(  173): enable_snd_device: sending audio calibration for snd_device(2) acdb_id(14)
D/ACDB-LOADER(  173): ACDB -> send_afe_cal
D/audio_hw_primary(  173): enable_snd_device: snd_device(2: speaker)
D/audio_hw_primary(  173): enable_audio_route: apply mixer path: low-latency-playback
D/audio_hw_primary(  173): start_output_stream: exit
I/Gecko   (  922): [Child 922] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../../content/html/content/src/HTMLAudioElement.cpp, line 235
I/Gecko   (  922): BrowserElementChildPreload - _waitForResult([object XrayWrapper [object Window @ 0xb279e940 (native @ 0xb33e6ad0)]])
I/Gecko   (  922): BrowserElementChildPreload - Entering modal state (outerWindowID=1, innerWindowID=3)
I/Gecko   (  922): BrowserElementChildPreload - Nested event loop - begin
I/Gecko   (  922): [Child 922] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../../content/html/content/src/HTMLAudioElement.cpp, line 235
I/Gecko   (  922): BrowserElementChildPreload - _waitForResult([object XrayWrapper [object Window @ 0xb279e940 (native @ 0xb33e6ad0)]])
I/Gecko   (  922): BrowserElementChildPreload - Entering modal state (outerWindowID=1, innerWindowID=3)
I/Gecko   (  922): BrowserElementChildPreload - Nested event loop - begin
I/Gecko   (  922): [Child 922] WARNING: NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome()) failed: file ../../../../../content/html/content/src/HTMLAudioElement.cpp, line 235
I/Gecko   (  922): BrowserElementChildPreload - _waitForResult([object XrayWrapper [object Window @ 0xb279e940 (native @ 0xb33e6ad0)]])
I/Gecko   (  922): BrowserElementChildPreload - Entering modal state (outerWindowID=1, innerWindowID=3)
I/Gecko   (  922): BrowserElementChildPreload - Nested event loop - begin

....

And if we eventually click OK we get
I/Gecko   (  922): BrowserElementChildPreload - recvStopWaiting(outer=1, inner=3, returnValue=true)
I/Gecko   (  922): BrowserElementChildPreload - recvStopWaiting [object XrayWrapper [object Window @ 0xb279e940 (native @ 0xb33e6ad0)]]
I/Gecko   (  922): BrowserElementChildPreload - Nested event loop - finish
E/GeckoConsole(  922): [JavaScript Error: "too much recursion" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 398}]

Comment 36

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #14)
> Andrew,
> 
> Can you please check and reassign appropriately?

Discussed in IRC - Fabrice or Vivien are people that could investigate this.

Fabrice - Would you be interested in looking into this bug?

Comment 37

[Gregor Wagner [:gwagner]]

Mrbkap looked at it yesterday and has more details about it. He thinks its a bug in the app itself.

Comment 38

[[:fabrice] Fabrice Desré]

(In reply to Gregor Wagner [:gwagner] from comment #37)
> Mrbkap looked at it yesterday and has more details about it. He thinks its a
> bug in the app itself.

Well, I see :
System JS : ERROR resource://gre/modules/BrowserElementParent.jsm:439 - InternalError: too much recursion
System JS : ERROR (null):0 - too much recursion
System JS : ERROR resource://gre/modules/BrowserElementParent.jsm:439 - unknown (can't convert to string)
System JS : ERROR (null):0 - too much recursion
System JS : ERROR (null):0 - too much recursion
System JS : ERROR (null):0 - too much recursion
System JS : ERROR (null):0 - too much recursion

as soon as we open the confirmation dialog, so we are broken too.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Fabrice Desré [:fabrice] from comment #38)
> as soon as we open the confirmation dialog, so we are broken too.

When you say "as soon as" what do you mean?

Looking at the source code of the application, here's its basic structure:

function main() { window.requestAnimationFrame(main, ...); tick(); }
function tick() { inputHandler.update(); }
inputHandler.update = (() => { if (inputState.clicked) clickedThing.handle(); inputState.reset(); });
clickedThing.handle = (() => { window.confirm(); });

Of note here: we requestAnimationFrame before calling confirm() and we clear our input state *after* the call to confirm. So, for every call to processNextEvent in confirm (or really _waitForResult), we call confirm again. This eats up the stack pretty fast and is, as far as I could tell yesterday, the reason for the "too much recursion" exceptions.

Comment 40

[Jason Smith [:jsmith]]

Note - I doubtful this is a bug in the app, as this has worked on 1.1 & 1.01 fine. It's not working 1.2+.

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

I think I see a potential problem. I'll debug this tomorrow.

In short, my explanation in comment 39 is correct, but we should be suppressing requestAnimationFrame events while the window is showing a modal dialog. This should hopefully be easy to fix.

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) from comment #41)
> In short, my explanation in comment 39 is correct, but we should be
> suppressing requestAnimationFrame events while the window is showing a modal
> dialog. This should hopefully be easy to fix.

This is exactly what's happening. I don't have an explanation for why the behavior would have changed and I can't find a combination of Gecko+Gaia from the working range to debug to see why things worked then.

Note that the behavior of continuing to fire animation frame events during a modal dialog is the same on desktop.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

Click on the button to see the behavior.

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: better testcase (careful: freezes Firefox)

This more closely imitates what the poppit app does. In particular, the key is the alert showing that we now recursively call the request animation frame callback.

I have no idea what to make of the regression range in comment 25. Looking at the code, this is almost certainly a regression from bug 731974. Jason, can you test trunk builds from before and after that bug landed (that would be changeset 513ec84b5c88)?

Also: for what it's worth, it's possible to see a kinder, gentler version of this bug even before bug 731974: with a working build, do the steps to reproduce here and then turn off and on the screen. The result should be one duplicated confirmation per screen cycle.

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

This fixes both the testcases in this bug to imitate Webkit and Blink.  It's a little scary that if there are other APIs that spin the event loop, this sort of bug will be possible with them. I'll take a look at that later.

Comment 46

[Jason Smith [:jsmith]]

(In reply to Blake Kaplan (:mrbkap) from comment #44)
> Created attachment 8337185 [details]
> better testcase (careful: freezes Firefox)
> 
> This more closely imitates what the poppit app does. In particular, the key
> is the alert showing that we now recursively call the request animation
> frame callback.
> 
> I have no idea what to make of the regression range in comment 25. Looking
> at the code, this is almost certainly a regression from bug 731974. Jason,
> can you test trunk builds from before and after that bug landed (that would
> be changeset 513ec84b5c88)?
> 
> Also: for what it's worth, it's possible to see a kinder, gentler version of
> this bug even before bug 731974: with a working build, do the steps to
> reproduce here and then turn off and on the screen. The result should be one
> duplicated confirmation per screen cycle.

That won't be possible - the oldest trunk builds we have don't go back to 2012.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8337189 [details] [diff] [review]
Possible fix

Make sure to push to try.

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

This appears to be orange on try. I'll look more tomorrow.

Comment 49

[(no longer active)]

(In reply to Blake Kaplan (:mrbkap) from comment #45)
> Created attachment 8337189 [details] [diff] [review]
> Possible fix
> 
> This fixes both the testcases in this bug to imitate Webkit and Blink.  It's
> a little scary that if there are other APIs that spin the event loop, this
> sort of bug will be possible with them. I'll take a look at that later.

I think the only APIs that can do that are alert(), confirm() and sync XHR.

(BTW, fantastic debugging here!)

Comment 50

[Andrew Overholt [:overholt]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #49)
> (BTW, fantastic debugging here!)

Big +1 from me on that!

Comment 51

[Alex Keybl [:akeybl]]

(In reply to Blake Kaplan (:mrbkap) from comment #48)
> This appears to be orange on try. I'll look more tomorrow.

Any updates? Please ask QA to help verify once we've got a working try build.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix for the orange

I'm about to send this to try, but this *should* fix the orange (I can't reproduce the orange locally). Basically, we can close a window between firing off the event to send delayed events and need to deal with that case.

This applies on top of attachment 8337189 [details] [diff] [review].

Comment 53

[Blake Kaplan (:mrbkap) (inactive)]

New try push is at https://tbpl.mozilla.org/?tree=Try&rev=f42bae68504b for those following along at home.

Comment 54

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77c1f23afc1c with a fix for the orange (I fixed some assertions in the test that was originally failing and had to update the assertion count.

Comment 55

[Blake Kaplan (:mrbkap) (inactive)]

Also of note (and sorry for the spam):

I didn't write a test because it's not immediately clear to me how to test a negative given our current test suite.

I squashed the two patches found here into a single patch because the first patch requires the second to not be orange.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/59672fad0777

https://tbpl.mozilla.org/php/getParsedLog.php?id=31347993&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31347658&tree=Mozilla-Inbound

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Green on try

Many thanks to Olli for his input here! This is green on try and fixes the (manual) testcases in this bug. The important bit is that we no longer send delayed events for documents that have navigated.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

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

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/567d2bba4f97

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/bb96292c9653

Comment 61

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage. Looks like this depends on bug 932309. Please attach a branch-specific patch.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1a484614916b

https://tbpl.mozilla.org/php/getParsedLog.php?id=31495004&full=1&branch=mozilla-b2g26_v1_2#error0

/usr/local/bin/ccache /builds/slave/m-b26_12-osx64-d-0000000000000/build/clang/bin/clang++ -o DOMStorageIPC.o -c  -fvisibility=hidden -DDOM_STORAGE_TESTS -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_MACOSX=1  -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/dom/base -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/content/events/src -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/ipc/chromium/src -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/dom/src/storage -I. -I../../../dist/include  -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/m-b26_12-osx64-d-0000000000000/build/obj-firefox/dist/include/nss      -fPIC  -Qunused-arguments  -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MP -MF .deps/DOMStorageIPC.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -Wno-error=uninitialized -Wno-error=deprecated-declarations -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -DNO_X11 -pipe  -DDEBUG -D_DEBUG -DTRACING -g -O3 -fno-omit-frame-pointer  -Werror  /builds/slave/m-b26_12-osx64-d-0000000000000/build/dom/src/storage/DOMStorageIPC.cpp
../../../dom/base/nsFocusManager.cpp:995:41: error: no member named 'IsCurrentInnerWindow' in 'nsPIDOMWindow'; did you mean 'GetCurrentInnerWindow'?
          !aDocument->GetInnerWindow()->IsCurrentInnerWindow()) {
                                        ^~~~~~~~~~~~~~~~~~~~
                                        GetCurrentInnerWindow
../../dist/include/nsPIDOMWindow.h:303:18: note: 'GetCurrentInnerWindow' declared here
  nsPIDOMWindow *GetCurrentInnerWindow() const
                 ^
1 error generated.

Comment 62

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add nsPIDOMWindow::IsCurrentInnerWindow for b2g26

This needs to be checked in before attachment 8342051 [details] [diff] [review] and with it that patch should be fine.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/59e38d576489
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a8ba3f906023

Comment 64

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/42b2a2adda8f for causing bug 946726.

Comment 65

[Kai-Chih Hu [:khu]]

Because 1.2 is going to be finished, should we mark this bug from koi+ to 1.3+? Thanks.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Backed out from b2g26 as well.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e535d93d88ad

Comment 67

[Blake Kaplan (:mrbkap) (inactive)]

I stared at this today and can't figure out what's going on. I can't reproduce it (and Shu couldn't either).

The easiest way to get this landed would be to use a more conservative API that only suppresses request animation frame callbacks while there's a modal dialog (I think that's pretty much guaranteed to not regress anything).

I'll try to debug this again tomorrow and see if I can turn anything up before giving up...

Comment 68

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 69

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rolled up patch for trunk

The important change here is to unsuppress events on mDoc for inner windows in FreeInnerObjects. It turns out that the sidebar, instead of removing the frame, navigates to about:blank and hides it. This change makes sure that we unsuppress events and get rid of the strong refs in the right places.

Comment 70

[Blake Kaplan (:mrbkap) (inactive)]

Let's try this again. Please note the green (several times over) try run pointed to by comment 68.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

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

Comment 72

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc leaks.
https://hg.mozilla.org/integration/b2g-inbound/rev/1366ee8c13c4

https://tbpl.mozilla.org/php/getParsedLog.php?id=32025933&tree=B2g-Inbound

Comment 73

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for orange on top of attachment 8347462

The previous patch managed to race with the patch for bug 949589, which exposed a bug in this code: we weren't properly handling mSuspendedDoc in all cases and with the fixed code in the in-content prefs pane we were leaking. In particular, we were playing fast and loose with which window owned which document. Adding to the confusion, we weren't traversing mSuspendedDoc, leading to even more leaks. I think this patch clarifies the situation.

I decided not to add mSuspendedDoc to cycle collection because we have a known lifecycle for it and I wanted this patch to be safe. In addition, the delayed events in the focus manager aren't cycle collected. Olli, should I file a followup to do that? bent seemed to think it made sense to *not* cycle collect them. Either way, this patch shouldn't leak anymore and we should be able to finally get this checked in.

I have a new try run at <https://tbpl.mozilla.org/?tree=Try&rev=c3e053a5ade2>.

Comment 74

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8348434 [details] [diff] [review]
Fix for orange on top of attachment 8347462 [details] [diff] [review]

This is orange on try...

Comment 75

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 76

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for orange on top of attachment 8347462

Olli, please see comment 73 for an explanation.

My previous patch tried to enforce the "mSuspendedDoc only on the outer window" assertion by asserting (and then forwarding) calls to Enter/LeaveModalState to the outer window. That didn't work. This approach works much better according to try.

Comment 77

[Preeti Raghunath(:Preeti)]

Blake,

In 12/17 triage we believe that this will remain a blocker for 1.2. Please land when appropriate.

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #73)
> I decided not to add mSuspendedDoc to cycle collection because we have a
> known lifecycle for it and I wanted this patch to be safe. In addition, the
> delayed events in the focus manager aren't cycle collected. Olli, should I
> file a followup to do that? bent seemed to think it made sense to *not*
> cycle collect them.
FocusManager is a service to cycle collecting stuff it owns isn't that useful.

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8348547 [details] [diff] [review]
Fix for orange on top of attachment 8347462 [details] [diff] [review]

We should make nsGlobalWindow::Enter/LeaveModalState() forward to outer window.
That seems to be what the code anyway always has when using those methods.

Could you please upload a full patch. Hard to understand this one.

Comment 80

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: As requested

Comment 81

[Blake Kaplan (:mrbkap) (inactive)]

Once more with feeling: https://hg.mozilla.org/integration/mozilla-inbound/rev/08dc60299942

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08dc60299942

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57157e39abea

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f9f4be5e8036
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c5eda4db2804

Comment 84

[Shu-yu Guo [:shu]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #83)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/57157e39abea
> 
> https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f9f4be5e8036
> https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c5eda4db2804

Why was this bug uplifted? It caused some regressions (the bugs that this one depends on) that aren't fixed yet.

Comment 85

[Yeojin Chung [:YeojinC]]

The fix was verified with the following devices:

1.4F Environmental Variables:
Device: Flame 1.4F MOZ
BuildID: 20140522000200
Gaia: 233dd43b3b976e66a619dbc1b4044ed1e3ca3e34
Gecko: c3c0c57daef8
Version: 30.0
Firmware Version: v10F-3

-----------------------------------------------------

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140522000200
Gaia: 233dd43b3b976e66a619dbc1b4044ed1e3ca3e34
Gecko: c3c0c57daef8
Version: 30.0
Firmware Version: v1.2-device.cfg

-----------------------------------------------------

1.4 Environmental Variables:
Device: Open_C 1.4 MOZ
BuildID: 20140522000200
Gaia: 233dd43b3b976e66a619dbc1b4044ed1e3ca3e34
Gecko: c3c0c57daef8
Version: 30.0
Firmware Version: v10F-3