1274966 - Broken browser e10s user agent tests because of ContentParent/ContentChild and BuildUserAgent timing

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=pine&filter-tier=1&filter-tier=2&filter-tier=3&fromchange=9785ffc4b1ad&selectedJob=28236

> 812 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_explicit_user_agent.html | The resulting user-agent is the navigator's UA - got "Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0", expected "Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 /"
> 814 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_explicit_user_agent.html | The user-agent is the navigator's UA - got "Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0", expected "Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 /"

Comment 1

[:gerard-majax]

Attachment: log on local browser m-c build

Comment 2

[:gerard-majax]

Attachment: log on local browser pine build

Comment 3

[:gerard-majax]

printf() in attachment 8755421 [details] shows that we have the child calling BuildUserAgent() BEFORE ContentChild::RecvAppInfo(). Hence, the proper AppInfo sent by the parent are not yet in the child. That is not the case on attachment 8755420 [details].

Comment 4

[:gerard-majax]

This is reproduced on pine since at least march 3rd, as one can check with: https://tools.taskcluster.net/task-inspector/#Q8_j9FnmRTq0NHitS-iI3A/

Comment 5

[:gerard-majax]

Attachment: Disable FetchPermissions() in child process r=...

After investigation, I have been able to see that the offending call was
related to nsPermissionManager::FetchPermissions(). I have not yet any proper
explanation of why, so I am sharing this for now.

Comment 6

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d989e58c3a

Comment 7

[Reuben Morais [:reuben]]

Looks like the race condition was caused by this cset, which uses the permission manager _very_ early in the initialization of the content process: http://hg.mozilla.org/projects/pine/rev/d5da8b62d465#l10.1

That code ends up calling FetchPermissions which does a synchronous SendReadPermission call to the parent process, and messes with the timing of the child seeing RecvAppInfo before nsHttpHandler::BuildUserAgent gets called.

My first impression is that we're abusing the permission manager for enabling formatToParts. I'll keep looking for a way to fix this. Attachment 8755590 [details] [diff] will probably regress bug 1129315 (do we care about that?).

Comment 8

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #7)
> Looks like the race condition was caused by this cset

Poor wording on my part here. That cset transformed the race condition into this bug, it didn't create the race condition.

Comment 9

[Reuben Morais [:reuben]]

Fabrice, as far as I can tell from the current Gecko on Pine + your Gaia pine branch on GitHub, the "previously-certified-app" permission test is essentially a test for the system app. Is that supposed to be extended in the future to other apps, or can we substitute the permission check for something like a hardcoded test on the principal?

Comment 10

[:gerard-majax]

Thanks reuben! But as you can see in comment 6, just removing FetchPermissions() call does break a lot of stuff.

Reverting the offending patch fixes the problem and is much more green, according to https://treeherder.mozilla.org/#/jobs?repo=try&revision=5364afea8669&filter-tier=1&filter-tier=2&filter-tier=3 but it breaks some features in Gaia (like, lockscreen clock).

Comment 11

[:gerard-majax]

I have a new tentative patch that just allows experimental formatsToPart() to anything with a system principal: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dabfeefdc19&filter-tier=1&filter-tier=2&filter-tier=3

Comment 12

[:gerard-majax]

Attachment: Allow experimental ECMA formatToParts in chrome r=...

MozReview-Commit-ID: 7qhHXYb7Acx

Comment 13

[:gerard-majax]

Comment on attachment 8755786 [details] [diff] [review]
Allow experimental ECMA formatToParts in chrome r=...

This seems good enough for getting mulet to work AND keeping browser test green. I have checked but the URI of the principal is either empty or "about:blank", so I don't have a better suggestion for now :/.

Comment 14

[:gerard-majax]

https://hg.mozilla.org/projects/pine/rev/32cd9c5dab22