1657231 - Improve URLPreloader corner cases

Status: RESOLVED FIXED

(Core :: XPConnect, task, P2)

Description

[Andrew McCreight [:mccr8]]

In looking over the code in the stacks in bug 1656973, I found a number of corner cases that the URLPreloader doesn't handle correctly when it gets accessed late in shut down. I think these issues all combine to cause the crash in bug 1656973, but they seem more like fall out from us calling into the ScriptPreloader at all during shut down, so maybe the fix for that crash should be in the script preloader.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1657231, part 1 - Only register the URL preloader as a reporter after it has been addrefed.

Normally this happens during initialization because we add the preloader
as an observer, but that seems fragile.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1657231, part 2 - Clear sInitialized when we destroy the singleton url preloader.

Some of the entry points into the URL preloader check sInitialized, and only
use it if it is true. However, if we're late in shutdown and we've already
cleared the singleton for the preloader then our failure to clear the
sInitialized flag means we'll recreate the preloader, which is going
to fail in various ways.

I think this won't help in the case of bug 1656973, because that seems
to go through AutoBeginReading, which unconditionally creates a new
singleton if one does not exist.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1657231, part 3 - Fail if add observer fails.

This can cause the URL preloader to think that initialization has succeeded
when it hasn't. This add observer call can fail if we're late in shutdown,
because the observer service still exists, but it isn't taking new
observers. I'm not sure how much it matters that we're failing to listen for
an observer that can't possibly fire at this stage, but a failure to check
for this seems to have contributed to the crash in bug 1656973, so we
might as well just fail.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1657231, part 4 - Don't allow using the URL preloader in the child.

It never gets initialized there anyways, so assert and clean up
a bit of dead code.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f4aab0ad04a
part 1 - Only register the URL preloader as a reporter after it has been addrefed. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e196f9cc2bb2
part 2 - Clear sInitialized when we destroy the singleton url preloader. r=kmag
https://hg.mozilla.org/integration/autoland/rev/73418f013a61
part 3 - Fail if add observer fails. r=kmag
https://hg.mozilla.org/integration/autoland/rev/caa4f187b037
part 4 - Don't allow using the URL preloader in the child. r=kmag

Comment 6

[Alexandru Michis [:malexandru]]

Backed out 4 changesets (Bug 1657231) for causing multiple xpcshell failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d51942b1e2d80cb9bfb1e5e5f21ba141e38fe065

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=xpcshell&tochange=d51942b1e2d80cb9bfb1e5e5f21ba141e38fe065&fromchange=9b48053f30035789d9c05aa2847c70b9bb892103&selectedTaskRun=dBp60DrIQEe09VLhjZFiaA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312321771&repo=autoland&lineNumber=4267

Comment 7

[Andrew McCreight [:mccr8]]

My part 1 changed the behavior so that sInitialized would still end up true even if initialization failed. Apparently that broke some XPCShell tests and a couple of startup cache gtests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7036d2098cef
part 1 - Only register the URL preloader as a reporter after it has been addrefed. r=kmag
https://hg.mozilla.org/integration/autoland/rev/68fee012fd52
part 2 - Clear sInitialized when we destroy the singleton url preloader. r=kmag
https://hg.mozilla.org/integration/autoland/rev/8a6c2d17d5c7
part 3 - Fail if add observer fails. r=kmag
https://hg.mozilla.org/integration/autoland/rev/c090bc84f79a
part 4 - Don't allow using the URL preloader in the child. r=kmag

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/7036d2098cef
https://hg.mozilla.org/mozilla-central/rev/68fee012fd52
https://hg.mozilla.org/mozilla-central/rev/8a6c2d17d5c7
https://hg.mozilla.org/mozilla-central/rev/c090bc84f79a

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 9168191 [details]
Bug 1657231, part 1 - Only register the URL preloader as a reporter after it has been addrefed.

Beta/Release Uplift Approval Request

• User impact if declined: This should fix the crashes in bug 1656973, though it feels like there must be some other changes that caused these very old bugs to start affecting people, so maybe we'll end up with other crashes. This is also a shut down crash, which probably has less impact on people. Maybe we could end up failing to save session store or something?
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This should only affect shutdown behavior. Probably the worst that can happen is we crash some other way.
• String changes made/needed: none

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9168191 [details]
Bug 1657231, part 1 - Only register the URL preloader as a reporter after it has been addrefed.

approved for 80.0b7

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c2717b21efb
https://hg.mozilla.org/releases/mozilla-beta/rev/50319133d024
https://hg.mozilla.org/releases/mozilla-beta/rev/f9f3480cefc7
https://hg.mozilla.org/releases/mozilla-beta/rev/3790f70a10fe