Closed Bug 1323366 Opened 7 years ago Closed 7 years ago

Crash in java.util.ConcurrentModificationException: at java.util.TreeMap$MapIterator.stepForward(TreeMap.java)

Categories

(Firefox for Android Graveyard :: General, defect)

Unspecified
Android
defect
Not set
critical

Tracking

(firefox52 wontfix, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: ting, Assigned: jwu)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-63ad3c50-4090-4c89-8ee1-58f472161214.
=============================================================
#2 top crash of Nightly 20161211030209 on Android, 4 crashes from single installation. There're 38 crashes in the past 6 months.
Because the TreeSet in IconRequest.java is modified(add/remove) from different threads, but it isn't thread safe. To support concurrency, we should replace TreeSet with ConcurrentSkipListSet.

Any concerns, Sebastian? I can fix this issue if you accept to use ConcurrentSkipListSet instead of TreeSet.
Assignee: nobody → topwu.tw
Flags: needinfo?(s.kaspari)
I think bug 1329439 also has same root cause. java.util.* should be replaced with java.util.concurrent.* to guarantee thread safe.
(In reply to Jing-wei Wu [:jwu] from comment #1)
> Because the TreeSet in IconRequest.java is modified(add/remove) from
> different threads, but it isn't thread safe. To support concurrency, we
> should replace TreeSet with ConcurrentSkipListSet.
> 
> Any concerns, Sebastian? I can fix this issue if you accept to use
> ConcurrentSkipListSet instead of TreeSet.

Interesting, I haven't heard of ConcurrentSkipListSet yet.

This would fix the issue here - but I think the underlying problem is a different one: IconTasks are executed on a single thread (see IconRequestExecutor) and therefore there shouldn't be concurrent access. However my IconRequestBuilder is cheating a bit: It's not actually constructing a IconRequest when calling build(). Instead it keeps a reference. What could happen is that we initiate an icon load and then new icon URLs come in (Tab.java) -> We do not build a new request but instead modify the existing one - that is currently in use. We actually might want to fix the builder here, I guess.
Flags: needinfo?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> However my IconRequestBuilder is cheating a bit: It's not actually
> constructing a IconRequest when calling build(). Instead it keeps a
> reference. What could happen is that we initiate an icon load and then new
> icon URLs come in (Tab.java) -> We do not build a new request but instead
> modify the existing one - that is currently in use. We actually might want
> to fix the builder here, I guess.

So we should create a new IconRequest when IconRequestBuilder calls build(), right? I agree that members in IconRequest shouldn't be modified by IconRequestBuilder after build() is called.
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Yeah, create a new request makes sense. Either the builder needs to remember all the values or we create a copy of the internal request in build(). Whatever you think is better here.

Watch out for IconRequest.modify() which copies the Object back into the builder for modification. This is used to modify an existing object and probably should not create a copy - Look at the code using it. Usually this code calls deferBuild() because we already have a request object and do not need to build one - maybe you can transform deferBuild() into something that modifies the actual object instead of building a new one - after refactoring the builder.
Flags: needinfo?(s.kaspari)
I use an internal IconRequest in IconRequestBuilder and copy a new one when build() is called. 

Also I think class that implements Preparer(AboutPagesPreparer, AddDefaultIconUrl, etc.) shouldn't call IconRequest.modify() because it only needs to change its members instead of creating a new IconRequestBuilder. That's why I create a new method IconRequest.icon(IconDescriptor) for it to modify icon set.
Comment on attachment 8826233 [details]
Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException,

https://reviewboard.mozilla.org/r/104218/#review105314

The icon code has a quite good test coverage with junit4 tests. If you change the behavior, can you add a test for it?

Maybe you can even add a test that triggers the exception and will be fixed by your change?

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java:145
(Diff revision 1)
> +     * Add an icon this request.
> +     */
> +    public void icon(IconDescriptor descriptor) {
> +        icons.add(descriptor);
> +    }

I think we might not need this, see comment below.

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java:137
(Diff revision 1)
> -        return request;
> +        IconRequest newRequest = internalRequest;
> +
> +        // Copy icon collection to prevent ConcurrentModificationException (Bug 1323366)
> +        newRequest.icons = new TreeSet<>(internalRequest.icons);
> +
> +        return newRequest;

You are creating another reference to the object but you are not really creating a copy. You are copying the tree set but after this method returns this builder will still reference the same request object that you returned.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/AboutPagesPreparer.java:32
(Diff revision 1)
> -            request.modify()
> +            request.icon(IconDescriptor.createLookupIcon(iconUrl));
> -                    .icon(IconDescriptor.createLookupIcon(iconUrl))
> -                    .deferBuild();
>          }

Those changes might not be needed: modify() copies the request into the builder, so we are actually modifying this object with the builder methods.
Attachment #8826233 - Flags: review?(s.kaspari) → review-
Comment on attachment 8826233 [details]
Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException,

https://reviewboard.mozilla.org/r/104218/#review110464

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java:145
(Diff revision 1)
> +     * Add an icon this request.
> +     */
> +    public void icon(IconDescriptor descriptor) {
> +        icons.add(descriptor);
> +    }

I've sent another patch and this change has be removed.

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java:137
(Diff revision 1)
> -        return request;
> +        IconRequest newRequest = internalRequest;
> +
> +        // Copy icon collection to prevent ConcurrentModificationException (Bug 1323366)
> +        newRequest.icons = new TreeSet<>(internalRequest.icons);
> +
> +        return newRequest;

I've sent another patch. The idea is to separate the reference of icons between IconRequestBuilder & IconRequest.

IconRequestBuilder keeps a internal IconRequest and create a new one when build() is call to prevent concurrent access.

For IconRequest#modify(), I don't create a new IconRequest in IconRequestBuilder but just keep the reference for any change to original object.

For the unit test, to command to try server I use is

```
./mach try -b do -p android-lint,android-api-15,android-x86,android-api-15-frontend -u robocop -t none
```

Doesn't it cover junit4 test? Or any arguments should I use for the test?

Also not sure what unit test should I add. Run two threads at same time to modify icons and see if ConcurrentModificationException occurs? Or simulate behavior of creating a tab? any advice is appreciated.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/AboutPagesPreparer.java:32
(Diff revision 1)
> -            request.modify()
> +            request.icon(IconDescriptor.createLookupIcon(iconUrl));
> -                    .icon(IconDescriptor.createLookupIcon(iconUrl))
> -                    .deferBuild();
>          }

This change is removed in new patch.
Comment on attachment 8826233 [details]
Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException,

https://reviewboard.mozilla.org/r/104218/#review111512

This looks good to me.

Btw. you can run the junit4 tests just from inside Android Studio.

Some ideas for test cases (TestIconRequestBuilder):
* Call build() twice on a builder and verify that the two objects are not the same
* After building call methods on the builder and verify that the previously build object is not changed
* Verify that using modify() (or the constructor) actually modifies the object in-place.
* You could add a test that iterates of the treeset and calls methods on the builder. Here's a sketch, this test fails with the current code and passes with your fix: https://pastebin.mozilla.org/8976342
Attachment #8826233 - Flags: review?(s.kaspari) → review+
What stops this from landing?
Flags: needinfo?(topwu.tw)
This issue still needs a junit test case and sorry I took too much time on investigating WebPagetest last week. I'll file the test case for review after onboarding.
Flags: needinfo?(topwu.tw)
(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> Btw. you can run the junit4 tests just from inside Android Studio.

I've added a test case for this bug. 

> Some ideas for test cases (TestIconRequestBuilder):
> * Call build() twice on a builder and verify that the two objects are not
> the same
> * After building call methods on the builder and verify that the previously
> build object is not changed
> * Verify that using modify() (or the constructor) actually modifies the
> object in-place.

I didn't test this because testSkipNetwork(), testSkipNetworkIf(), etc. should already cover modify() behavior. 

> * You could add a test that iterates of the treeset and calls methods on the
> builder. Here's a sketch, this test fails with the current code and passes
> with your fix: https://pastebin.mozilla.org/8976342

There is a try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68df7d1ce9cc91cb25f8e0062980ebeea8cbcdcf

and the junit test result
https://public-artifacts.taskcluster.net/fOvjZ3qjRSO2bxyTDs_7sQ/0/public/android/unittest/automationDebug/classes/org.mozilla.gecko.icons.TestIconRequestBuilder.html
Keywords: checkin-needed
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/750d9da36fc7
Create new IconRequest to prevent ConcurrentModificationException, r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/750d9da36fc7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
IIUC, this signature goes back to older versions as well? Do we need to backport this to 53 as well? (Too late for 52 at this point)
Flags: needinfo?(topwu.tw)
Comment on attachment 8826233 [details]
Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException,

Yes, looks like it should be uplifted to 53 as well.

Approval Request Comment
[Feature/Bug causing the regression]: IconRequest doesn't support concurrent operations within different threads.
[User impact if declined]: App crash
[Is this code covered by automated tests?]: Yes, there is a junit test case covered this code.
[Has the fix been verified in Nightly?]: Yes, review by :sebastian
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The code just creates new IconRequest to prevent concurrent operation on same object. It doesn't change business logic.
[String changes made/needed]: No
Flags: needinfo?(topwu.tw)
Attachment #8826233 - Flags: approval-mozilla-aurora?
Comment on attachment 8826233 [details]
Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException,

Fix a crash. Aurora53+.
Attachment #8826233 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.