Prevent view ID conflict/crash by multiplying each generated view ID by a prime number

[sufyanAbbasi]

Fixes #2493

• Increase the chances of uniqueness for generated View IDs by multiplying by a randomly chosen, 5-digit prime number in order to prevent crashes due to generateViewId() producing an ID number that is the same as an ID number assigned to a View in the GoogleMapsFragment.
  • I think this occurs because generateViewId() is not thread safe when GoogleMapsFragment views are also being produced at the same time.

• Created a survey via npm run test:e2e:create in ground-platform to create a survey to repro breakage.
• Run local server via npm run start:local by modifying package.json to use test/data-create
• Opened survey, verified that crash was avoided.

@gino-m, @shobhitagarwal1612 PTAL!

[scolsen]

LGTM but I do have some questions.

1. Do we need to do it this way at all? The other task fragments don't call add and instead use bindings (see e.g. MultipleChoiceTaskFragment).
2. If we need to use add here, can we either drop the LinearLayout container or define it directly in the layout for the fragment? add claims that the ID is optional and using an argument of 0 will just add it without using a parent—can we use the existing build-time ID of the general task fragment parent instead, to avoid this problem entirely?

[gino-m]

Nice sleuthing, @sufyanAbbasi ! Looks like some checks are failing?

[sufyanAbbasi]

@scolsen:

Do we need to do it this way at all? The other task fragments don't call add and instead use bindings (see e.g. MultipleChoiceTaskFragment).

If we need to use add here, can we either drop the LinearLayout container or define it directly in the layout for the fragment? add claims that the ID is optional and using an argument of 0 will just add it without using a parent—can we use the existing build-time ID of the general task fragment parent instead, to avoid this problem entirely?

Thank you so much for the alternatives! I have a strong feeling these would actually work, now understanding better what the root cause of it is. Unfortunately, I'm under time constraints and will take me a little bit to investigate and verify that the fix actually works. Can I schedule some time with you to peer program on this?

@gino-m:

Looks like some checks are failing?

* What went wrong:
Execution failed for task ':ground:kaptGenerateStubsDevStagingUnitTestKotlin'.
> Could not resolve all files for configuration ':ground:detachedConfiguration83'.
   > A build operation failed.
         Could not read workspace metadata from /workspace/.gradle/caches/transforms-4/041b4d9711d44f92419df7bc9f3bdaac/metadata.bin
      > Could not read workspace metadata from /workspace/.gradle/caches/transforms-4/041b4d9711d44f92419df7bc9f3bdaac/metadata.bin

I can't tell if these are just transient, I'll try rerunning.

[sufyanAbbasi]

@scolsen and I had a chance to meet, we determined that due to the nature of needing to render a view to return from createTaskBody() to be added to addTaskView(), we need to create the dummy LinearLayout to achieve that. Static IDs doesn't work either since we run into the same issue of ID conflicts. Finally, we could refactor the entire codebase to return a Fragment instead of a View there, then handle the Fragment add upstream, but that's far more work than it is worth.

Hence, we have decided to just go for this solution for now with comment.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.77%. Comparing base (cb73c59) to head (df241c4).

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #2588   +/-   ##
=========================================
  Coverage     55.77%   55.77%           
  Complexity     1317     1317           
=========================================
  Files           317      317           
  Lines          7130     7130           
  Branches        886      886           
=========================================
  Hits           3977     3977           
  Misses         2672     2672           
  Partials        481      481           

Flag	Coverage Δ
service	55.77% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...tion/tasks/location/CaptureLocationTaskFragment.kt	95.83% <100.00%> (ø)
.../datacollection/tasks/point/DropPinTaskFragment.kt	50.00% <100.00%> (ø)
...tacollection/tasks/polygon/DrawAreaTaskFragment.kt	60.37% <100.00%> (ø)