Closed Bug 1274966 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(3 files, 1 obsolete file)

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 /"
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].
This is reproduced on pine since at least march 3rd, as one can check with: https://tools.taskcluster.net/task-inspector/#Q8_j9FnmRTq0NHitS-iI3A/
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.
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?).
(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.
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?
Flags: needinfo?(fabrice)
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).
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
MozReview-Commit-ID: 7qhHXYb7Acx
Attachment #8755590 - Attachment is obsolete: true
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 :/.
Attachment #8755786 - Flags: review?(fabrice)
Attachment #8755786 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/projects/pine/rev/32cd9c5dab22
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(fabrice)
Resolution: --- → FIXED
Assignee: nobody → lissyx+mozillians
Blocks: 1287107
You need to log in before you can comment on or make changes to this bug.