Closed Bug 884394 Opened 11 years ago Closed 11 years ago

Notes+ app crashes upon resource transfer

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Milestone:
1.2 C4(Nov8)
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: ranbena, Assigned: matias)

References

Details

(Keywords: perf, Whiteboard: [c=memory p= s= u=1.2] [b2g-crash][MemShrink:P2])

Attachments

(4 files, 1 obsolete file)

b2g-info binary
26.50 KB, application/x-executable
Details
Archive containing memory analysis
46.94 KB, application/x-gzip
Details
Post-patch memory analysis
55.46 KB, application/x-gzip
Details
Screen Shot 2013-09-25 at 11.36.12 AM.png
125.74 KB, image/png
Details 
      No description provided.
Summary: Notes+ app crashes upon a large resource transfer → Notes+ app crashes upon resource transfer
The Notes+ app can be found here https://marketplace.firefox.com/app/notesplus
All the code is on our GitHub repo https://github.com/EverythingMe/ffos-notes/tree/phase2/

Basically, after logging into the app with an Evernote account, a sync process begins.
The communication with the Evernote server is done using Evernote JS SDK https://github.com/evernote/evernote-sdk-js which implements the Thrift protocol and exchanges data using ArrayBuffers.

The API transferring Note, Notebook and Resource data. In a normal sync process, the app receives what Notes, Notebooks and Resources had "changed" and require retrieval. Then, every type of data is requested and after retrieval, stored using indexedDB.

Notes and resources under ~750KB are retrieved, stored and displayed successfully.

The bug
=======
When retrieving a resource over ~750KB, the app freezes for a few seconds and eventually crashes (the app does, not Gaia). This happens also with Notes with textual content exceeding ~750KB. Meaning, it's not an image display issue.

Bugs reproduce on
=================
Unagi
Firefox OS Simulator
adb shell dmseg on Unagi:
<4>[06-18 07:40:25.291] [2897: Notes+]select 2897 (Notes+), adj 2, size 30625, to kill
<4>[06-18 07:40:25.291] [2897: Notes+]send sigkill to 2897 (Notes+), adj 2, size 30625
I'm going to guess this is a OOM. Can you get a dump of about memory here?
Please include the revisions of Gecko and Gaia that you tested on, in this and all bugs.

On the call we had, someone mentioned that the crash occurs before we even start storing the data in indexeddb.  If this is correct, it sounds to me like the thrift deserialization might be using a lot of memory and/or generating a lot of garbage.  How did you determine that indexeddb isn't involved?

It would be helpful to find a minimal crashing testcase.  If all we need to do is call into Evernote and then deserialize the message, that would rule out a lot of other possibilities.

Unfortunately Unagi is not the best device to test on, because it does not have low-memory notifications (bug 876610).  These notifications might cause us to GC and save ourselves when we're about to crash.  If you have access to another device, it would be helpful to test there as well.  All of the other devices have working low-memory notifications, as far as I know.

I'm still very confused by the fact that this is crashing the FFOS simulator.  That doesn't sound like an OOM issue, even though the dmesg output in comment 2 is clearly an OOM.  The simulator crash might be an unrelated issue.  I'd like to test this locally, but I need the username and password for this test evernote account.

I'm not quite finished writing the guide to OOM debugging, but I think it's far enough along to be of use to you: https://wiki.mozilla.org/B2G/Debugging_OOMs.
Severity: normal → critical
Keywords: crash
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [b2g-crash]
Ok I tested on the simulator with the "matias_evme" account given in the email thread, and while it does not crash back to the homescreen like it does on the device (Unagi flashed with a releng 1.1 build from 6/17), it does "crash" in the sense that there is a javascript error and the sync does not complete:

Content JS LOG at app://{bc7129da-8666-2e49-bda1-b2c5ca051db0}/js/evernote.js:90 in anonymous: [FxOS-Notes] processXHR xhr.responseText: <html>
<head>
<meta charset="utf-8" />
<meta http-equiv="X-UA-Compatible" content="IE=9,chrome=1" />
<meta name="viewport" content="width=device-width,initial-scale=1" />
<link rel="Shortcut Icon" href="/favicon.ico" type="image/x-icon" />
<link rel="stylesheet" href="/redesign/global/css/reset.css" />

<link rel="stylesheet" href="/redesign/global/css/fonts.css" media="all" />

<link rel="stylesheet" href="/redesign/global/css/header.css" />
<link rel="stylesheet" href="/redesign/global/css/layout.css" />

<title>Evernote Error</title>
</head>
<body>
  <div class="header">
    <div class="logo-bar">
      <a class="evernote-logo" href="http://evernote.com/">Evernote</a></div>
  </div>
  <div id="container-boundingbox" class="wrapper">
    <div id="container" class="wrapper">
      <div class="main">
        <div class="page-header">
          <h1>
            Oops, we encountered an error.</h1>
        </div>
        <div>
          <p>
            Sorry, we've encountered an unexpected error.</p>
        </div>
        <div class="clear"></div>
      </div>
    </div>
    <div class="shadow wrapper">
      <img src="/redesign/global/img/desktop-shadow-full.png" />
    </div>
    <div class="footer wrapper">
      <a class="footer-entry" href="http://evernote.com/tos/">Terms of Service</a><a class="footer-entry" href="http://evernote.com/privacy/">Privacy Policy</a><span class="footer-entry last">Copyright 2013 Evernote Corporation. All rights reserved.</span>
    </div>
  </div>
</body>
</html>
www.evernote.com : server does not support RFC 5746, see CVE-2009-3555
Timestamp: 6/18/13 1:57:53 PM
Error: NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMHTMLDivElement.removeChild]
Source File: app://system.gaiamobile.org/js/popup_manager.js
Line: 92
> Error: NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 
> (NS_ERROR_INVALID_POINTER) [nsIDOMHTMLDivElement.removeChild]
> Source File: app://system.gaiamobile.org/js/popup_manager.js
> Line: 92

This looks unrelated, but it also looks like it might be important.  Can you file a separate bug for this, please?  Please cc alive and timdream.
I tried to do get_about_memory against the phone. If I do it too soon, then the memory usage looks fine, but if I wait longer then the app crashes while get_about_memory is running, and get_about_memory never completes.

Next, I will try on a device that supports memory pressure events.
b2g-info will tell us if the unagi is set up to use 256 or 512mb of RAM.  If it's the former, you can re-flash and unlock the extra memory; that would probably help.

https://intranet.mozilla.org/B2G_Team/Unagi
> https://intranet.mozilla.org/B2G_Team/Unagi

Hm, I don't see the link to the 512mb kernel anymore.  I'm not sure where it is, but if you need it, mwu will know.
I'm pretty sure I had flashed this phone with the 512 kernel, but I don't see the b2g-info script to verify.
> I don't see the b2g-info script to verify.

It landed in the past few days, so you need to resync and rebuild.  I'll attach it to the bug for you here.
Attached file b2g-info binary — 
Push this to /system/bin, then adb shell chmod 777 /system/bin/b2g-info
Keywords: perf
Whiteboard: [b2g-crash] → u=MarketPlace , [b2g-crash]
If the app crashes before you can get a memory report, perhaps you could decrease the size of the resource you're fetching.
I've determined that indexeddb isn't involved because the last console.log i see is the call to the thirft api asking for the note and i never see the console.log with the response, so it's not even getting to the point of inserting into indexeddb.

(In reply to Justin Lebar [:jlebar] from comment #4)
> Please include the revisions of Gecko and Gaia that you tested on, in this
> and all bugs.
> 
> On the call we had, someone mentioned that the crash occurs before we even
> start storing the data in indexeddb.  If this is correct, it sounds to me
> like the thrift deserialization might be using a lot of memory and/or
> generating a lot of garbage.  How did you determine that indexeddb isn't
> involved?
> 
> It would be helpful to find a minimal crashing testcase.  If all we need to
> do is call into Evernote and then deserialize the message, that would rule
> out a lot of other possibilities.
> 
> Unfortunately Unagi is not the best device to test on, because it does not
> have low-memory notifications (bug 876610).  These notifications might cause
> us to GC and save ourselves when we're about to crash.  If you have access
> to another device, it would be helpful to test there as well.  All of the
> other devices have working low-memory notifications, as far as I know.
> 
> I'm still very confused by the fact that this is crashing the FFOS
> simulator.  That doesn't sound like an OOM issue, even though the dmesg
> output in comment 2 is clearly an OOM.  The simulator crash might be an
> unrelated issue.  I'd like to test this locally, but I need the username and
> password for this test evernote account.
> 
> I'm not quite finished writing the guide to OOM debugging, but I think it's
> far enough along to be of use to you:
> https://wiki.mozilla.org/B2G/Debugging_OOMs.
We have made a few tests with simple text notes (without image resources) and we've found out that if the size of the text is larger then ~750kb the app crashes when trying to fetch the note, so it might be what you said in a previous comment about the thrift deserialization using a lot of memory.

(In reply to Justin Lebar [:jlebar] from comment #13)
> If the app crashes before you can get a memory report, perhaps you could
> decrease the size of the resource you're fetching.
Assignee: nobody → jhylands
Status: NEW → ASSIGNED
> Archive containing memory analysis

Okay, this is great; it tells us exactly what's going on.

> 85.61 MB (100.0%) -- explicit
> ├──60.65 MB (70.85%) -- js-non-window
> │  ├──56.14 MB (65.58%) -- gc-heap
> │  │  ├──51.00 MB (59.57%) ── unused-chunks

unused-chunks are pieces of memory that can be gc'ed, but these aren't being collected.

This seems like a pretty clear gc tuning bug.
Flags: needinfo?(anygregor)
Whiteboard: u=MarketPlace , [b2g-crash] → u=MarketPlace , [b2g-crash][MemShrink]
Can we impose a hard limit on how much unused-chunks JS keeps around on B2G?  There are no circumstances under which we can afford to waste more than 5mb or so of chunks.  50mb is just crazy.
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink] → u=MarketPlace , [b2g-crash][MemShrink:P2]
As a point of comparison, I believe jemalloc imposes a hard limit of 1 or 2mb on its chunk cache, which serves the same purposes as JS's unused-chunks.
(In reply to Justin Lebar [:jlebar] from comment #17)
> > Archive containing memory analysis
> 
> Okay, this is great; it tells us exactly what's going on.
> 
> > 85.61 MB (100.0%) -- explicit
> > ├──60.65 MB (70.85%) -- js-non-window
> > │  ├──56.14 MB (65.58%) -- gc-heap
> > │  │  ├──51.00 MB (59.57%) ── unused-chunks
> 
> unused-chunks are pieces of memory that can be gc'ed, but these aren't being
> collected.
> 
> This seems like a pretty clear gc tuning bug.

We used to have a value that determined how many chunks we keep around and additional chunks were returned right away. I can't find it any more :( We should definitely limit the number of chunks we keep around on b2g.
Flags: needinfo?(anygregor)
Attached patch patch (obsolete) — — Splinter Review 
Maybe something like this would help here.
For B2G and Android we only keep 2 chunks around and for desktop browser 30.
Attachment #767668 - Flags: feedback?(wmccloskey)
Attachment #767668 - Attachment is patch: true
Attachment #767668 - Attachment mime type: text/x-patch → text/plain
Is there an evernote account I can use for testing?
Gregor - I applied the patch (had to do one file manually - I'm running in v1-train/b2g18), and re-ran the memory analysis, which is attached. Clearly fixed the GC issue, but the app still OOM'ed. I pulled the memory analysis just a couple seconds before it crashed. Here's the dmesg output:

<4>[06-26 12:59:53.372] [9543: Notes]select 9616 ((Preallocated a), adj 6, size 4616, to kill
<4>[06-26 12:59:53.372] [9543: Notes]send sigkill to 9616 ((Preallocated a), adj 6, size 4616
<6>[06-26 13:00:05.544] [3498: GonkSensors][ 9543] 19543  9543    56596    24237   0       2           134 Notes
<3>[06-26 13:00:05.544] [3498: GonkSensors]Out of memory: Kill process 9543 (Notes) score 649 or sacrifice child
<3>[06-26 13:00:05.544] [3498: GonkSensors]Killed process 9543 (Notes) total-vm:226384kB, anon-rss:96920kB, file-rss:28kB
Flags: needinfo?(justin.lebar+bug)
Flags: needinfo?(anygregor)
Note that I rebooted the phone, and tried again, and this time it didn't crash (no OOM). It took a long time (many minutes) to sync the 5 entries from my evernote account. Gregor, if you want access to run some tests yourself, I can give you the info.
If this patch doesn't unquestionably solve the problem (sounds like it), we should file a new bug for that patch (which we really want anyway) and track this bug separately.
Flags: needinfo?(justin.lebar+bug)
Ran, can you try the attached patch, and see if it fixes your OOM crash issue?
Flags: needinfo?(ran)
Attachment #767668 - Flags: feedback?(wmccloskey) → review?(wmccloskey)
We should also figure out why we allocate 56MB here.
Flags: needinfo?(anygregor)
Ok this patch doesn't solve the problem here.
I see that we allocate 250! MB for the demo profile.
I will move my patch to another bug.

Bytes Used	Count		Symbol Name
 251.03 MB      99.9%	990	 	                      nsThread::ProcessNextEvent(bool, bool*)
  94.24 MB      37.5%	404	 	                       mozilla::dom::indexedDB::AsyncConnectionHelper::Run()
  94.24 MB      37.5%	398	 	                        mozilla::dom::indexedDB::AsyncConnectionHelper::OnSuccess()
    848 Bytes    0.0%	6	 	                        mozilla::dom::indexedDB::IDBRequest::NotifyHelperCompleted(mozilla::dom::indexedDB::HelperBase*)
  91.98 MB      36.6%	59	 	                       nsInputStreamReadyEvent::Run()
  64.81 MB      25.8%	526	 	                       nsTimerEvent::Run()
     32 Bytes    0.0%	1	 	                       mozilla::dom::indexedDB::FinishTransactionRunnable::Run()
     48 Bytes    0.0%	2	 	                 __CFRunLoopDoObservers
Blocks: 887652
Attachment #767668 - Attachment is obsolete: true
Attachment #767668 - Flags: review?(wmccloskey)
No longer blocks: 887652
Depends on: 887652
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink:P2] → [u=MarketPlace] [b2g-crash][MemShrink:P2]
Flags: needinfo?(ran)
Any other clues why we allocate 250 meg for the demo profile?
Hello team

We as well as our partners are keen to progress work on the app — and there doesn't seen to be any movement for a few days.

As a check-in I wondered if we are anywhere closer to solving for the issues at hand and being able to re-test Notes+.

Thanks for all your work on this. It's greatly appreciated.
Is this required to be fixed in order to have the notes packaged app with evernote integration work properly? If so, then yes, this should be leo+.
Marking as a blocker (along with bug 887652) since this impacts a 1.1 preinstalled app and the STR will be a common use case.
blocking-b2g: --- → leo+
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
Any other ideas on what might be causing this?
Status: ASSIGNED → NEW
We know that indexedDB is part of the problem.  See comment 28.
Comment 1 said that the app exchanges data using ArrayBuffers. Are these being transferred between threads? (Either cross-thread or cross-process?) Alternatively, are typed arrays being stored in IndexedDB for this?

I'm asking because we've run into problems like this when using small typed arrays that are views of large ArrayBuffers. Previously, only the data within the typed array view would be copied. After bug 789593, the *entire* ArrayBuffer gets copied, and the receiving typed array is recreated as a (small) view on that ArrayBuffer. This is per spec, but it causes problems when the user is not expecting this behavior. The usual fix is to create a new typed array to copy the relevant window of data from the original:

  var buffer = ArrayBuffer(999999);
  var orig_view = Uint8Array(buffer, 400, 64);
  var view_for_transmission = Uint8Array(orig_view);
  postMessage(...view_for_transmission...); // Or store into IndexedDB or whatever

This may be a total red herring; I haven't looked to see what's actually going on here. It's just a fairly common way to inadvertently create very large temporary allocations.
That does sound like a reasonably likely culprit. Thank you very much, Steve.
Hey guys,
a few days ago the login stopped working, we were using the same method the twittershare app uses and the cloudfoundry site stopped working, we have changed the login to use manifest redirects and it now works again, I have uploaded a new version to the Firefox Marketplace, alternatively you can get the app from github https://github.com/EverythingMe/ffos-notes/tree/phase2 the branch we're working on is phase2. keep in mind the app is privileged and has to be installed into a device in order for the login to work correctly.
I have tested with the latest Gecko build and with the latest master branch and still the app crashes before i even get to handle the object and store in IndexedDB.
you can edit the file evernote.js on line 387 is where i call the method self.getNote() which is requesting the note from Evernote and you can see the callback is never called therefor not added to IndexedDB and the app crashes.
I noticed that you don't have the 'storage' permission in your manifest file. It allows you to store more than 5MB.

https://hg.mozilla.org/mozilla-central/file/18467a85acf6/dom/apps/src/PermissionsTable.jsm#l213
I've added the storage permission, just one question about it, what is the substitute for? how exactly do i use it?

Like i said in Comment 38 the app crashes before i can insert to IndexedDB so the permission didn't solve the issue.
Matias, does comment 35 help in any way? It seems like a likely culprit with an easy workaround.
Taras, per Justin's comment #34 we believe IndexedDB to be contributing to this problem. Would it make sense to have anyone in your team look into this issue?
Flags: needinfo?(taras.mozilla)
(In reply to Mike Lee [:mlee] from comment #42)
> Taras, per Justin's comment #34 we believe IndexedDB to be contributing to
> this problem. Would it make sense to have anyone in your team look into this
> issue?

Sounds like some intersection of Steve Fink and Justin Lebar would be most effective at this.
Flags: needinfo?(taras.mozilla)
Flags: needinfo?(sphink)
Flags: needinfo?(justin.lebar+bug)
> Sounds like some intersection of Steve Fink and Justin Lebar would be most effective at this.

I can't get to this until we fix some other outstanding B2G memory leaks.

Sorry.
Flags: needinfo?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #44)
> > Sounds like some intersection of Steve Fink and Justin Lebar would be most effective at this.
> 
> I can't get to this until we fix some other outstanding B2G memory leaks.
> 
> Sorry.

Justin will you document the bugs this is dependent on?
Flags: needinfo?(justin.lebar+bug)
We don't know what bugs this is dependent on; that's part of the investigation that needs to be done.

Maybe I'm misunderstanding you?

Leo is cherry-picking fixes from b2g18, and based on the bugs they have and haven't taken, it seems pretty unlikely to me that Leo is going to take a fix here.  Before we scramble to fix this, can we check with them whether this is strongly desired?
Flags: needinfo?(justin.lebar+bug)
Renom per comment 46.
blocking-b2g: leo+ → leo?
Triage - triaging partners for leo do not feel a need to block on this. Over to Mozilla triage.

(In reply to Justin Lebar [:jlebar] from comment #46)
> We don't know what bugs this is dependent on; that's part of the
> investigation that needs to be done.
> 
> Maybe I'm misunderstanding you?
> 
> Leo is cherry-picking fixes from b2g18, and based on the bugs they have and
> haven't taken, it seems pretty unlikely to me that Leo is going to take a
> fix here.  Before we scramble to fix this, can we check with them whether
> this is strongly desired?
Whiteboard: [u=MarketPlace] [b2g-crash][MemShrink:P2] → [u=MarketPlace] [b2g-crash][MemShrink:P2] [leo-triage]
Not blocking then based on comment 48 - ni? on Karen to make sure she sees this.
blocking-b2g: leo? → -
Flags: needinfo?(kward)
Status: NEW → ASSIGNED
Whiteboard: [u=MarketPlace] [b2g-crash][MemShrink:P2] [leo-triage] → [c=memory u=1.1] [b2g-crash][MemShrink:P2] [leo-triage]
Handing this bug over to the MemShrink team per recent B2G discussions. Doug, please reassign within your team as makes sense.

From: "Doug Turner" <dougt@mozilla.com>
To: "Andreas Gal" <gal@mozilla.com>, "Faramarz Rashed" <frashed@mozilla.com>, "Justin Lebar" <justin.lebar@gmail.com>, mlee@mozilla.com, "Lucas Adamski" <ladamski@mozilla.com>, "Johnny Stenback" <jst@mozilla.com>
Sent: Friday, August 2, 2013 5:12:46 PM
Subject: memshrink-b2g

Justin is responsible for memshrink related activities in B2G.  If you see bugs in triage, please make Justin and myself aware of them...
Assignee: jhylands → doug.turner
Assignee: doug.turner → mrbkap
I spent the past couple of days looking at this bug. What I've found so far:
  - The OOM killer definitely steps in, but it *doesn't* kill the Notes+ app anymore. So far, I've seen it kill every other app (including the homescreen) but it's leaving the foreground app alone.

  - The notes are making it into the IndexedDB database, though the app has a lot of trouble displaying them (I verified this by pulling the database off of the phone and inspecting it locally using (https://addons.mozilla.org/en-us/firefox/addon/indexeddb-browser/)

  - On a similar note, the app doesn't get appear to always get a response from the IndexedDB API (as was pointed out in comment 14).

The last point seems like the most promising lead in terms of debugging, so I'll start there next week.
Oh, one more data point: I've yet to be able to get an about:memory dump with any gigantic strings hanging around, so it seems possible that we're GC'ing the JS string relatively quickly and using the memory up somewhere else (like in the IPC layer or somesuch).
Hello team. 

Any updates here? 

Both Telefonica & Telenor are now pushing really hard for this app (Notes+ with Evernote integration) to land.
blocking-b2g: - → koi?
Flags: needinfo?(kward)
Blake is on PTO until September 19th so there won't be any movement here until he gets back.  IIRC from my last conversation with him it looked like IndexedDB events were being lost somewhere, but we hadn't figured out if it was a problem with the app or Gecko.  While the app is using a lot of memory it doesn't appear that it is it getting killed (other apps on the system are getting killed to free up space for Notes+).

Also now that Justin has left I am the new point of contact for memory related issues in b2g.
Flags: needinfo?(sphink)
Flags: needinfo?(khuey)
Hi Kyle, Can yu provide an update on this? It is very important for Apps - Specifically the Notes app.
One thing that would make this bug a lot easier to test would be if there was a version of the application that we could test in the app store. Especially for the tools that Gregor uses (and the state of desktop-b2g on mac). Ran, can you look into that?
Flags: needinfo?(khuey) → needinfo?(ran)
Correct me if I am wrong, but there is:

https://marketplace.firefox.com/app/notesplus
Blake, Donovan is correct - that's the app.
We've kept it in the marketplace (although the negative user feedback) for testing purposes.
Flags: needinfo?(ran)
Gregor, can you try to use your magic tools on that app (I don't think IPC is implicated anymore)?
Flags: needinfo?(anygregor)
I tried it on b2g desktop. I couldn't find the app via the marketplace but I could launch it via the browser itself.
Once I want to log in via evernote I see a 404 page (see attachment).
Flags: needinfo?(anygregor)
blocking-b2g: koi? → koi+
Whiteboard: [c=memory u=1.1] [b2g-crash][MemShrink:P2] [leo-triage] → [c=memory p= s= u=1.2] [b2g-crash][MemShrink:P2]
Target Milestone: 1.1 QE5 → ---
(Donovan, please see comment 60)

I've been debugging this for a couple of days and here's my current state:

As far as I can tell, we always successfully import the given notebooks into IndexedDB. The test account that I've been using has two notebooks: "My notebook" and "matias-doit9's notebook". I can always (I've never had any problem) load "My notebook" and load the single note in it (title "Image") that has an image in it. I think the note with the image is about the same size as the text-only note, but I'm not sure.

When I try to load the other notebook, we successfully receive the note from IDB; however, when we try to generate the snippet to display in the main pane, something odd happens generating the HTML from the ENML that we're storing: we appear to stop generating the HTML (in xml-writer.js) but very early on: that is to say that the size of the string generated so far is only about 9k-20k (for comparison's sake, the Image note ends up being about 90k of HTML, including the image in base64). I didn't hit a breakpoint in js_ReportOutOfMemory, but I don't know if we're guaranteed to go through there whenever we run out of memory. That *seems* to suggest that we're using up a lot of process memory in C++, but I don't know that for sure yet. I'll try to figure that out tomorrow.
Flags: needinfo?(dpreston)
Matias, in comment 37 you mentioned cloudfoundry breaking and switching to using the redirect in the manifest functionality; in comment 60 it looks like something is broken with cloudfoundry again, do you have any idea what happened?
blocking-b2g: koi+ → koi?
Flags: needinfo?(dpreston) → needinfo?(matias)
Target Milestone: --- → 1.1 QE5
I don't think we're running out of memory. I think we're getting a slow script dialog...
With the patch in bug 921519, this should "work." That being said, loading the note is still extremely slow. A major cause of the slowness is that the XML parser spends way too much time looking for < characters. Once that's fixed (I have a pull request open on the library being used at <https://github.com/berryboy/enml-js/pull/7>) things seem to work rather speedily.

Another pull request I'd thought I'd mention was that clicking on "Trashed notes" when the trash is empty is currently broken <https://github.com/EverythingMe/ffos-notes/pull/26> fixes that.
Donovan, regarding Comment 60 if he used the version which is available on the marketplace its an old version, the latest version which uses the manifest redirect method is still pending approval.

Blake, I'll check the pull request and merge it and re submit the app to the marketplace.
Flags: needinfo?(matias)
Back to koi+ because I think Donovan made a mistake.
blocking-b2g: koi? → koi+
I just tested, and with my pull requests and the patch in bug 921519, I can successfully import and view the notes in the test account. I'm going to mark this bug FIXED because of that.

That being said, things are still pretty laggy when interacting with the app, so we're definitely going to want to profile and tighten up its performance. I'm going to open a followup to track that work.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blake, in order to test it we need a Gecko version with your patch in it I assume.
Does it already reside in the Nightly builds?
(In reply to Ran Ben Aharon (Everything.me) from comment #68)
> Does it already reside in the Nightly builds?

Hi Ran, the patch got checked into FirefoxOS v1.2 today, so it should be in tomorrow's nightly (if I understand the markings in the bug correctly).
Great news. Thanks a lot for your help Blake.
There was a misunderstanding. After speaking with Blake, we realized we were using the wrong accounts. Blake's fixes work for demo account 'matias-doit9'. On a Hamachi on Koi, the crash still occurs with Demo account 'matias_evme'. 

At the moment, the bug is not reproducible through the simulator and must occur on device. Bug occurs on B2G revision a8a44b5b5ce8c100882feb524913caf3da2b26c3. Installation must occur through Nightly's App Manager and not through the marketplace. Per Comment 65, still waiting on marketplace app to be updated.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As Mason mentions, the app crashes when trying to sync with the matias_evme demo account but not with the matias-doit9 account.
Got the app to load on the FFOS Simulator! Using Aurora on Mac, install the App Manager - https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_the_App_Manager. 

Clone the ffos-notes locally to disk. Add as a packaged app the ffos-notes app. Start the simulator, and it works! With the simulator, I am able to at least login with the 'matias_evme' account. 

A Fresh simulator uses 110 mb of memory on my Macbook Pro according to activity monitor. After logging into evernote, the B2G Process takes 210 MB, which means the syncing takes ~100 mb if activity monitor is accurate. The syncing process also stutters on my Macbook Pro for a couple of seconds. 

Finally, clicking on the "demo notebook" does not work in the simulator, but produces a memory leak. After clicking on the demo notebook, an Range Error JS error occurs, and clicking  back on "All notes" shoots B2G memory usage up to 320 mb, much of which gets reclaimed in a few seconds. Memory usage then reduces to 200 mb.
I've submitted the latest version with Blake's pull requests to the marketplace, version 2.0.6 pending approval.

As Mason mentions in Comment 71 when trying to sync the matias_evme demo account the app crashes, this happens in the latest version as well, the account mastias-doit9 works with no problem.
(In reply to Matias Crivolotti from comment #74)
> As Mason mentions in Comment 71 when trying to sync the matias_evme demo
> account the app crashes, this happens in the latest version as well, the
> account mastias-doit9 works with no problem.

I spent the past two days looking into the crash. There are two crashes: one that seems easy to fix (see <https://github.com/EverythingMe/ffos-notes/pull/27>) and one that will require a bunch more work.

The first crash happens when importing the account from the internet and we crash (as has been noted) before we even call into any IndexedDB APIs (aside from opening the DB, that is). The cause of the crash is that we end up storing the image internally in a format that weighs in around 28mb. We then try to make two copies of the data in short order and that uses up the rest of the memory available to us and we crash. My pull request eliminates the need for the two extra copies (as well as a third temporary copy in debug-only code) and allows us to import the note.

The second crash will be much more difficult to fix. When we select the notebook, we attempt to generate some sort of string for the content of the resources (I have to admit, I'm not entirely sure what the code is attempting to accomplish). Given that we're starting with a 28mb string, building the second string, which is twice as long (!) is guaranteed to run out of memory. That step completes successfully on desktop, but concatenating an array of 14000000 elements via Function.prototype.apply won't work. Handling resources needs to use less memory overall and Evernote.enml2html needs a major overhaul. I wonder if it's possible to store the resources as blobs and do something fancy with the Gaia redirect functionality to avoid having to store the images as text at all?

I'm happy to discuss this more, but I'm not going to have time to write the code, so giving this bug over to Dees to follow up.
Assignee: mrbkap → dchinniah
I've merged all of Blake's code and re-submitted the app to the marketplace as version 2.0.7.

Thanks Blake for all the help on this issue.

(In reply to Matias Crivolotti from comment #76)
> I've merged all of Blake's code and re-submitted the app to the marketplace
> as version 2.0.7.
> 
> Thanks Blake for all the help on this issue.

Matias - I've requested the review team to look at and approve the latest version.

However, are you and the team able to look at the second crash issue (comment 75) that Blake mentions. It seems that this is something that could be looked at on your end before anything further can be done on the platform end.
I believe one problem with the second issue is that the Evernote library is by Evernote, so it will be harder to change. Matias can correct me if I am wrong.
Thanks!
HI Desigan,

Is there any progress on this bug? Who needs to review the patch?
Flags: needinfo?(dchinniah)
Target Milestone: 1.1 QE5 → 1.2 C3(Oct25)
(In reply to Preeti Raghunath(:Preeti) from comment #80)
> HI Desigan,
> 
> Is there any progress on this bug? Who needs to review the patch?

Hi Preeti.

We are awaiting feedback from the E.me team (Ran + Matias)
https://bugzilla.mozilla.org/show_bug.cgi?id=884394#c77
Flags: needinfo?(dchinniah)
Assignee: dchinniah → matias
Hi all,
I have been modifying the app to work with blake's patches and using window.URL.createObjectURL from the image blob and not converting it to a base64 string and im happy to say i successfully managed to sync the 2 demo accounts.

New app version is 2.1 and it is submitted to the marketplace and waiting approval.

Thanks a lot Blake for all the help.
Matias, that is fantastic news!
Great. Passing v2.1 onto the review team. More soon.
Notes+ v2.1 is now approved and live on the Marketplace:
https://marketplace.firefox.com/app/notesplus/

Could we please run it through it's paces again.
I just tested both demo accounts, matias_evme, and matias-doit9 on v1.2 with an otoro device. I was able to successfully login with both accounts!

However, both demo accounts do not seem to have the same notebooks as they did in my previous testing which did crash the app. Before, both demo accounts had notebooks of lorum ipsum text (I don't recall how much). Matias, do you remember what text and notes were in the accounts before the fix?

As another test, I added a lorum ipsum text note to the matias-doit9 account. It contains 50 paragraphs, 4544 words, 30900 bytes of Lorum Ipsum. This extra note had no effect on login crashing as I could login!

Some basic performance numbers:
matias_evme login -> touch all notes -> notes showing up -> ~10.5 seconds
matias-doit9 login -> touch all notes-> notes showing up -> ~8 seconds
touch lorum ipsum -> notes showing = ~3 seconds

One other bug I noticed. If I do a clean install of the notes+ app from the marketplace, then try to login with either account, the sync process will continue indefinitely until I touch on "all notes" or anything else. The account's notebook never shows up until I touch a different notebook. During this process, the UI shows that the sync process is working, but nothing ever occurs. I waited for > 2.5 minutes.

If I log out of an account, then re log back in, everything shows up as intended. The previous bug only occurs on a clean install + first login to evernote.
Flags: needinfo?(matias)
Renom - we've already shipped 1.1 with this problem, which means we probably can't block on this at this point.
blocking-b2g: koi+ → koi?
Also - not a crash, this is an OOM.
Keywords: crash
I believe 1.1 shipped with Notes, not Notes+?
According to Gregor, this is fixed now, so closing.
Status: REOPENED → RESOLVED
blocking-b2g: koi? → koi+
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
(In reply to Mason Chang [:mchang] from comment #86)

> One other bug I noticed. If I do a clean install of the notes+ app from the
> marketplace, then try to login with either account, the sync process will
> continue indefinitely until I touch on "all notes" or anything else. The
> account's notebook never shows up until I touch a different notebook. During
> this process, the UI shows that the sync process is working, but nothing
> ever occurs. I waited for > 2.5 minutes.

Ran/Matias — Can we do any progressive download or syncing here? Is there a UI showing x% of 100% completed/synced?

 
> If I log out of an account, then re log back in, everything shows up as
> intended. The previous bug only occurs on a clean install + first login to
> evernote.

Ran/Matias — Could we please look at this specifically? This is exactly what happened to Evernote on their first run of testing the last time around, and they refused to look any further.
(In reply to Desigan Chinniah from comment #91)
> 
> Ran/Matias — Can we do any progressive download or syncing here? Is there a
> UI showing x% of 100% completed/synced?

We can do that. 
  
> Ran/Matias — Could we please look at this specifically? This is exactly what
> happened to Evernote on their first run of testing the last time around, and
> they refused to look any further.

We'll look into that.
Flags: needinfo?(matias)
Any in-roads, team? We'd really like to get a new version into the hands of Evernote for testing but want to make sure that they are getting a pretty good experience.
Dees, you should probably open new bugs for any issues you still want looked at -- this bug is closed.
Setting Target Milestone for this sprint's koi+ fixes.
Target Milestone: 1.2 C3(Oct25) → 1.2 C4(Nov8)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: