Closed Bug 1204559 Opened 6 years ago Closed 7 months ago

Remove org.json.simple from android-sync code base

Categories

(Firefox for Android Graveyard :: Android Sync, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: vivek, Unassigned, Mentored)

References

Details

(Whiteboard: [lang=java][bad first bug])

User Story

This bug will track the remaining changes from https://bugzilla.mozilla.org/show_bug.cgi?id=1182193#c0

* converting ExtendedJSONObject to use org.json.  There are API mismatches here, so this takes manual effort.

* mopping up the last few org.json.simple uses (JSONArray, etc).  Again, this takes manual effort.

Attachments

(8 files, 6 obsolete files)

38.90 KB, image/png
Details
46.96 KB, image/png
Details
32.63 KB, image/png
Details
204.54 KB, image/png
Details
142.74 KB, image/png
Details
117.94 KB, image/png
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
No description provided.
Assignee: nobody → vivekb.balakrishnan
User Story: (updated)
User Story: (updated)
Bug 1204559 - org.json.JsonArray related changes r?nalexander

Ignored 5 tests that are related to JSONArray to List<JSONObject> conversion.
They wil be fixed with org.json.JSONObject changes.
Attachment #8665633 - Flags: review?(nalexander)
Comment on attachment 8665633 [details]
MozReview Request: Bug 1204559 - org.json.JsonArray related changes r?nalexander

Bug 1204559 - org.json.JsonArray related changes r?nalexander

Ignored 5 tests that are related to JSONArray to List<JSONObject> conversion.
They wil be fixed with org.json.JSONObject changes.
Bug 1204559 - org.json.JsonObject related changes r?nalexander
Attachment #8667504 - Flags: review?(nalexander)
Will this also fix the instant crash when enabling sync on firefox for android on some phones (mostly phones with newer 64 bits mediatek processors)?
(In reply to Tom Levels from comment #4)
> Will this also fix the instant crash when enabling sync on firefox for
> android on some phones (mostly phones with newer 64 bits mediatek
> processors)?

That's the hope and the motivation for this work.
Status: NEW → ASSIGNED
Bug 1204559 - org.json.JsonArray related changes r?nalexander

Ignored 5 tests that are related to JSONArray to List<JSONObject> conversion.
They wil be fixed with org.json.JSONObject changes.
Attachment #8679035 - Flags: review?(nalexander)
Bug 1204559 - org.json.JsonObject related changes r?nalexander
Attachment #8679036 - Flags: review?(nalexander)
Comment on attachment 8665633 [details]
MozReview Request: Bug 1204559 - org.json.JsonArray related changes r?nalexander

These patches are good but too risky to take all at once.  We may be able to come back to this approach in the future.
Attachment #8665633 - Flags: review?(nalexander)
Comment on attachment 8667504 [details]
MozReview Request: Bug 1204559 - org.json.JsonObject related changes r?nalexander

These patches are good but too risky to take all at once.  We may be able to come back to this approach in the future.
Attachment #8667504 - Flags: review?(nalexander)
Comment on attachment 8679035 [details]
MozReview Request: Bug 1204559 - DO NOT LAND - Debug logging, take 1. r?nalexander

These patches are good but too risky to take all at once.  We may be able to come back to this approach in the future.
Attachment #8679035 - Flags: review?(nalexander)
Comment on attachment 8679036 [details]
MozReview Request: Bug 1204559 - org.json.JsonObject related changes r?nalexander

These patches are good but too risky to take all at once.  We may be able to come back to this approach in the future.
Attachment #8679036 - Flags: review?(nalexander)
What will be the approach now to fix this bug?
(In reply to Tom Levels from comment #12)
> What will be the approach now to fix this bug?

Hi Tom!  I have a device with an MTK6752 processor ordered; it should arrive in the next week.  That processor appears to reproduce the issue, so our approach is to try to fix the third-party JSON library.  If we can't reproduce, we'll evaluate getting additional devices, or putting more time into this approach.
Hi Nick, great to hear that you will get a device with which you will very likely be able to reproduce the issue. Otherwise I would be happy to help you (as far as I can). Anyway great to hear that I will be able to Sync my mobile device again some time in the future.
(In reply to Tom Levels from comment #14)
> Hi Nick, great to hear that you will get a device with which you will very
> likely be able to reproduce the issue. Otherwise I would be happy to help
> you (as far as I can). Anyway great to hear that I will be able to Sync my
> mobile device again some time in the future.

Hi Tom, thanks for the offer.  I was working closely with a bug reporter several months ago, who isolated the problem this far.  Unfortunately, I just ran out of stuff to test -- the json-simple library is failing allocating memory in an unexpected way, and it looks like we need real access to the device to dig deeper.

Once we have a lead, we'll appreciate you verifying the issue and hopefully the fix.
I have an Xperia E4G based on the MediaTek MT6732 and I see the crash constantly after setting up sync.

If you need help with testing let me now.

Here is one of my crashes:
https://crash-stats.mozilla.com/report/index/698a6f28-9a4c-47c5-909d-005722151115
(In reply to Mogens Isager from comment #16)
> I have an Xperia E4G based on the MediaTek MT6732 and I see the crash
> constantly after setting up sync.
> 
> If you need help with testing let me now.

Thanks, Mogens!  I expected the device to arrive Thursday, but it's still not here :(
(In reply to Nick Alexander :nalexander from comment #17)
> (In reply to Mogens Isager from comment #16)
> > I have an Xperia E4G based on the MediaTek MT6732 and I see the crash
> > constantly after setting up sync.
> > 
> > If you need help with testing let me now.
> 
> Thanks, Mogens!  I expected the device to arrive Thursday, but it's still
> not here :(

My device arrived, but I can't (yet?) replicate the crash :(

Mogens, Tom: could you install the CPU-z App (https://play.google.com/store/apps/details?id=com.cpuid.cpu_z) and screenshot the SoC, Device, and System  screens?  I think there's nothing sensitive, so you can attach to this ticket; but if you prefer, you can email them to me.

Others following this ticket: half a dozen such reports could be helpful, if you have the time.  I was so confident this would be a 64-bit/Octacore issue, but perhaps it's not...
Flags: needinfo?(misager)
Flags: needinfo?(TomLevels)
Nicolas: over in https://bugzilla.mozilla.org/show_bug.cgi?id=964854#c22, you provided a crash report.  (You also did a lot of debugging with me -- thank you for that!)  If you still have that device, could you also use CPU-z to ID it?  I'd appreciate you verifying that the issue still happens, too.

Thanks!
Flags: needinfo?(nicolas.croiset)
Attached image Xperia E4G, CPU-Z SoC
Flags: needinfo?(misager)
Nicolas, Mogens: this bug is ridiculous.  Chasing this yet further, it rather looks like we're failing to allocate memory, although I can't confirm that yet.

The following is a custom Fennec build, with debugging turned up to 11.  Please use |adb install| to install it, and then add a *TEST* Firefox Account and start capturing |adb logcat -v threadtime| logs while you try to Sync.

It's very important that you *NOT* use your production Firefox Account, since the logs will contain complete login details, etc.  I suggest using an email like "testXXXX@mockmyid.com"; you can go to http://www.mailinator.com/inbox.jsp?to=testXXXX to complete the verification loop.  (Replace XXXX with your own random identifier.)

Since the login details are in the logs, please email them to me rather than attaching them.

Thanks!

http://people.mozilla.org/~nalexander/fennec-test1.apk
sha1 4f1dded0f0deeac91f2922ca723d29e28909951d
Flags: needinfo?(misager)
Comment on attachment 8679035 [details]
MozReview Request: Bug 1204559 - DO NOT LAND - Debug logging, take 1. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23347/diff/1-2/
Attachment #8679035 - Attachment description: MozReview Request: Bug 1204559 - org.json.JsonArray related changes r?nalexander → MozReview Request: Bug 1204559 - DO NOT LAND - Debug logging, take 1. r?nalexander
Attachment #8679035 - Flags: review?(nalexander)
Attachment #8679036 - Attachment is obsolete: true
Attached image CPU screenshot SOC
Flags: needinfo?(TomLevels)
Attached image CPU screenshot device
Attached image CPU screenshot system
OK, I have gotten several logs from Nicolas and Mogens -- thank you both.  Thank you Tom -- you appear to be running *exactly* the same chipset that I have in this Meizu m1 note (although a very different version of Android).

This puzzle is quite confusing.  I've pushed yet another APK for testing to http://people.mozilla.org/~nalexander/fennec-test3.apk, logs appreciated.

This one has single-thread JSON parsing and tests the JSON parser built with JFlex 1.5.0.  rnewman noticed that there were some bugs fixed between JFlex 1.4.2 and 1.5.0; let's see if y'all are hitting them.

If not, we'll log the steps of the JFlex DFA.  Thanks, everybody!
My testing friends, thank you for all of your logs.  My personal bet right now is wandering between "bad state in the lexer", "concurrency issue in Fennec", and "unicode encoding issue".  To get a bead on some of this, I've created a small test App.  Since I want this to be reproducible, and since nobody should trust binaries from the interwebs, I've pushed the source to

https://github.com/ncalexan/Bug1204559

You can find the (unsigned, debug) binary at

https://rawgit.com/ncalexan/Bug1204559/0.1/binaries/Bug1204559-0.1.apk

When you run this, you'll see a text input pre-filled with some JSON (known to fail, although you can edit it.  I think you'll see failure with '{}' too).  Tap "Parse" -- green is success, red is failure.

There is copious logging -- this is running a jflex 1.5.1 parser with debug output on.  That should show a little about what's happening.

If you could capture logs (no need to keep them private) for the prefilled input and just '{}' (i.e., delete everything, type open and close curly parens), that would be great.  Thank you both!
(In reply to Nick Alexander :nalexander from comment #30)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eb30b6b89be

Nicolas, thank you for the logs.  Nicolas's logs showed no issues parsing JSON, so we have a Fennec-specific issue.  I posit:

1) concurrency;
2) locale handling;
3) Activity interaction.

The try build linked above includes some testing right around the locale initialization code.  It shouldn't have concurrency issues.  Could I get logs when launching Fennec, then killing the process, then launching Fennec again?  Thanks!

To use the try build go to

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eb30b6b89be&selectedJob=14133286

and then choose the link in the left corner to "Build".  You should see an APK there.  That APK has package name org.mozilla.fennec, *not* org.mozilla.fennec_nalexander.  If you have a Nightly on the device, you'll need to remove it.  (I can produce a local build if that's a problem, but I wanted to track these test builds better by pushing to Mozilla's mercurial servers and try servers, etc.)

Thanks!
> and then choose the link in the left corner to "Build".  You should see an
> APK there.

To clarify, the APK is at

http://archive.mozilla.org/pub/mobile/try-builds/nalexander@mozilla.com-e86516b91617ccf3a242c70a70288665ef5fdd14/try-android-api-11/fennec-45.0a1.en-US.android-arm.apk

  That APK has package name org.mozilla.fennec, *not*
> org.mozilla.fennec_nalexander.

Here I was unclear: I meant the package name that gets installed on your Android device.

The custom packages I build have had package name org.mozilla.fennec_nalexander.  This one has org.mozilla.fennec.  This is relevant because:

  If you have a Nightly on the device, you'll
> need to remove it.  (I can produce a local build if that's a problem, but I
> wanted to track these test builds better by pushing to Mozilla's mercurial
> servers and try servers, etc.)
Comment on attachment 8679035 [details]
MozReview Request: Bug 1204559 - DO NOT LAND - Debug logging, take 1. r?nalexander

This didn't actually need review, I just didn't know how to push to review without flagging somebody (in this case, myself).
Attachment #8679035 - Flags: review?(nalexander)
Is there more debugging needed for this bug?
(In reply to Tom Levels from comment #34)
> Is there more debugging needed for this bug?

No, we just need to get to it.  Sorry!
Assignee: vivekb.balakrishnan → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(nicolas.croiset)
Flags: needinfo?(misager)
Priority: -- → P3
Did a first pass of this locally. Converted the EOJ, and some of the API mismatches. Nothing compiles yet, but overall this seems like a nice "background bug" to chug along.
Some Friday afternoon progress; mostly mechanical changes, it's slowly getting there. Yet to convert unit tests.

The tricky/tedious part of this - verifying that it all still functions correctly beyond the obvious, nulls are handled properly, etc - is still to do.
Product: Android Background Services → Firefox for Android
Getting closer. A few TODOs in the code, and four failing tests.

It appears as if ArrayList's equals is misbehaving, but it must be something else:
>org.mozilla.android.sync.test.TestCommandProcessor > testParseCommandNullArg FAILED
>    java.lang.AssertionError at TestCommandProcessor.java:116

The rest are failing because we still need a proper "toJsonString" implementation for the new EOJ:
> org.mozilla.gecko.browserid.test.TestJSONWebTokenUtils > testGetPayloadString FAILED
>    org.junit.ComparisonFailure at TestJSONWebTokenUtils.java:146
> org.mozilla.gecko.browserid.test.TestJSONWebTokenUtils > testDSAGeneration FAILED
>    org.junit.ComparisonFailure at TestJSONWebTokenUtils.java:133
> org.mozilla.gecko.browserid.test.TestJSONWebTokenUtils > testRSAGeneration FAILED
>    org.junit.ComparisonFailure at TestJSONWebTokenUtils.java:85
Refactored + equals/hashcode with a stack instead of recursion.
Attachment #8927515 - Attachment is obsolete: true
Attachment #8958231 - Attachment is obsolete: true
Attachment #8679035 - Attachment is obsolete: true
Attachment #8667504 - Attachment is obsolete: true
Attachment #8665633 - Attachment is obsolete: true
Attachment #8913895 - Attachment is obsolete: true
Attachment #8958231 - Attachment is obsolete: false
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.