When using a custom profile, Fennec can be initialized with a different profile

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

Trunk
Firefox 24
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

More testing woes :(

The new session restore tests (bug 874985) are frequently failing on 2.2 devices because they aren't receiving the session restore RestoreEnd event: https://tbpl.mozilla.org/?tree=Try&rev=f5aac3003174

After checking the screenshots, it looks like they aren't doing the restore to begin with. Looking at the logs, the failed tests are showing this:

06-06 23:18:29.766 D/BRN     ( 1898): PATH: /data/data/org.mozilla.fennec/files/mozilla/f69e41l4.default/sessionstore.js
06-06 23:18:29.766 D/BRN     ( 1898): ERROR: sessionFile doesn't exist

whereas the passing tests look like this:

06-06 23:25:00.862 D/BRN     ( 6359): PATH: /mnt/sdcard/tests/profile/sessionstore.js
06-06 23:25:00.862 D/BRN     ( 6359): file was read

Unless I'm mistaken, the profile should always be in /mnt/sdcard/tests/profile. Not sure if this is a bug with GeckoProfile or the test initialization scripts.
Assignee

Updated

6 years ago
Blocks: 874985
I pulled out the 6 of my 10 remaining hairs on my head 6 months ago tracking down a similar problem.  Messing with the BaseTest.java.in::setup() method was the cause that I found.  I see that you are doing the same thing as well here:
https://hg.mozilla.org/try/rev/a9064c871847

Is it possible that the code in that patch linked above is failing or causing setup to not load the profile?
Assignee

Comment 2

6 years ago
(In reply to Joel Maher (:jmaher) from comment #1)
> I pulled out the 6 of my 10 remaining hairs on my head 6 months ago tracking
> down a similar problem.  Messing with the BaseTest.java.in::setup() method
> was the cause that I found.  I see that you are doing the same thing as well
> here:
> https://hg.mozilla.org/try/rev/a9064c871847
> 
> Is it possible that the code in that patch linked above is failing or
> causing setup to not load the profile?

I think this may be a problem already in the tree. Looking at other unrelated failures (such as https://tbpl.mozilla.org/?rev=34b36e6d4369&tree=Try), I see things like the following:

On a normal test:
06-06 04:26:21.671 D/GeckoHealthRec( 5016): Looking for /mnt/sdcard/tests/profile/times.json

On a failing test (failing with bug 870918):
06-06 04:18:00.304 D/GeckoHealthRec( 4585): Looking for /data/data/org.mozilla.fennec/files/mozilla/3i2cggt6.default/times.json

Using the wrong profile/database is a very plausible cause for bug 870918, so I'm convinced that this bug is responsible for other failures as well.
Blocks: 870918
Assignee

Comment 3

6 years ago
Printing the stack trace in the logs, I found that the initialization for BrowserProvider/TabsProvider is happening before the activity is created. That's fine, except for the fact that we actually initialize the profile in those classes' onCreate() methods. Since we don't have the custom profile path we're giving to the activity yet, those classes are initializing GeckoProfile with the default profile...and then chaos ensues.

This profile initialization code in providers was added way back in bug 708651. At the time, we had a race between profile creation and using the profile, where there was a NPE if the latter occurred first. Now, GeckoProfile creates the profile lazily, so there's no risk of calling GeckoProfile.get() and having a non-existent profile. Consequently, the code here is no longer necessary (and is in fact detrimental).

Try results for new testSessionRestoreOOMRestore test, before this patch:
https://tbpl.mozilla.org/?tree=Try&rev=fdd2bf2ca216

Try results for new testSessionRestoreOOMRestore test, after this patch:
https://tbpl.mozilla.org/?tree=Try&rev=b01433ffad30

Try results for bug 870918, before this patch:
https://tbpl.mozilla.org/?tree=Try&rev=71c4908a3a2c

Try results for bug 870918, after this patch:
https://tbpl.mozilla.org/?tree=Try&rev=e63dce5d7b77

Try results for all tests after this patch:
https://tbpl.mozilla.org/?tree=Try&rev=f9e22b7bd191

At minimum, this bug is responsible for the two bugs listed here, but I'm curious to see what else might be fixed in the product/test framework.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #760157 - Flags: review?(gbrown)
Assignee

Updated

6 years ago
Summary: Wrong profile directory can be used during robocop tests → When using a custom profile, Fennec can be initialized with a different profile
Wow, this would be great if a handful of our random bugs were solved by this!  Fantastic!

I looked at the try server run for the entire set of tests (the last link from above):
https://tbpl.mozilla.org/?tree=Try&rev=f9e22b7bd191

I don't see a patch queue history in the tbpl view to indicate that there is something fixed.  Maybe this change is landed, or maybe it is a relic of try server goodness where you had pushed that patch to a previous try run and it counts it as already landed on the tree.

Could you confirm that the tryserver run for all tests includes the patch?
(In reply to Joel Maher (:jmaher) from comment #4)
> Wow, this would be great if a handful of our random bugs were solved by
> this!  Fantastic!
> 
> I looked at the try server run for the entire set of tests (the last link
> from above):
> https://tbpl.mozilla.org/?tree=Try&rev=f9e22b7bd191
> 
> I don't see a patch queue history in the tbpl view to indicate that there is
> something fixed.  Maybe this change is landed, or maybe it is a relic of try
> server goodness where you had pushed that patch to a previous try run and it
> counts it as already landed on the tree.
> 
> Could you confirm that the tryserver run for all tests includes the patch?

Sometimes HG (or Try) puts the changeset in the "parent", so for this Try run:
https://tbpl.mozilla.org/?tree=Try&rev=f9e22b7bd191

We have this cset:
https://hg.mozilla.org/try/rev/f9e22b7bd191

But the parent cset is:
https://hg.mozilla.org/try/rev/39f8c22833f2

Which has the patch
Nice work Brian. CC'ing Richard in case this affects Sync code or the way Sync can cause the providers to be created.
Comment on attachment 760157 [details] [diff] [review]
Remove GeckoProfile.get() from provider creation

Review of attachment 760157 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great find. Thanks for all the try runs -- very thorough.
Attachment #760157 - Flags: review?(gbrown) → review+
Interesting!

Assuming that I'm understanding your analysis, I think that removing these calls is not the right fix.

It's possible for Sync (or, indeed, system-wide search integration) to be the first thing that runs and touches the CPs, where 'touches' means actually doing work that opens databases.

That will eventually cause getDatabaseHelperForProfile(null, false) to be called, regardless of this patch, which will fall back to the default profile. (Or we'll pass in the profile name; same problem.)

You cannot assume that your activity will run before ContentProvider init. ContentProvider lifecycle can be long -- longer than multiple different activity runs. The same ContentProvider can be resident in memory and running queries while you browse with your profile, launch five webapps, set up and discard Sync, etc. That's why the CPs are set up to handle multiple profiles at the same time.

It should be perfectly safe to call GeckoProfile.get *at any time*, with any profile argument, without screwing up a subsequent activity launch.

If it isn't, it means that the Firefox process can only 'host' a single profile -- we can't run a sync at the same time as we're running a web app, because the sync will touch the CPs, the CPs will touch GeckoProfile, and so will the browser, right?

Am I misunderstanding the problem?
(Not that it's bad to remove these calls -- I mean that removing these calls does not eliminate the problem, because you're just pushing these calls back to the first CP read or write.)
Assignee

Comment 10

6 years ago
(In reply to Richard Newman [:rnewman] from comment #8)
> 
> It should be perfectly safe to call GeckoProfile.get *at any time*, with any
> profile argument, without screwing up a subsequent activity launch.
> 
> If it isn't, it means that the Firefox process can only 'host' a single
> profile -- we can't run a sync at the same time as we're running a web app,
> because the sync will touch the CPs, the CPs will touch GeckoProfile, and so
> will the browser, right?

Yeah, I should have mentioned that this is a stop-gap solution for the failing tests and not a proper fix. I was focused on getting these robocop tests passing, and this should address those failures since robocop shouldn't be triggering any external CP requests. We should have this patch anyway as you mentioned in comment 9, but there's more work to be done to fix the underlying bug here.
Assignee

Comment 11

6 years ago
Filed bug 881378 as a follow-up. I'll take brief look, but I'm long overdue to work on some gamepad bugs.
Target Milestone: --- → Firefox 24
https://hg.mozilla.org/mozilla-central/rev/1a7469edb07e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.