1250871 - `content` can be null when messages are still received in the frame script

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

The global `content` object is something frame scripts rely. In bug 1229195, however, we found that our infrastructure is reporting runs where it's possible for `content` to null/ de-initialized during shutdown whilst messages from the messageManager are still arriving.

There are numerous try runs and orange flagged (debug) builds that offer plenty proof that this may happen.

Note that the object that is the handler for the messages is also an nsIWebProgressListener, which may or may not explain why the lifetime discrepency might occur.

This might not be the correct product/ component, but I didn't know of a better one.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Is this about e10s or non-e10s or both?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

And is this a regression from Bug 1126089, which explicitly made us keep message managers connected later (but docshell is destroyed a bit before them)?


For in-process case nsAsyncMessageToChild::Run() in nsFrameLoader could check that mDocShell is non-null.
So change if (tabChild && tabChild->GetInnerManager()) {
to 
if (tabChild && tabChild->GetInnerManager() && mDocShell) {


e10s case
TabChild::RecvAsyncMessage could check that !mDestroyed.

Anyone want to test those?

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #1)
> Is this about e10s or non-e10s or both?

Still looking, but so far I only see non-e10s intermittent failures.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #2)
> For in-process case nsAsyncMessageToChild::Run() in nsFrameLoader could
> check that mDocShell is non-null.
> So change if (tabChild && tabChild->GetInnerManager()) {
> to 
> if (tabChild && tabChild->GetInnerManager() && mDocShell) {

This doesn't look unreasonable to me, certainly. I'll give it a push or two.

Comment 5

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7678f86d6c

Comment 6

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c39c7f1cffc

Comment 7

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77dc3c65fcc8

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: only send messages to child scripts when the docShell is still around

Well, if Try is happy, I'm happy.

Olli, apologies for upping your review load even further - but hey, you practically suggested this change! ;-)

Like I mentioned earlier, the e10s case apparently doesn't need additional guards.

Comment 9

[Mike de Boer [:mikedeboer]]

After this is fixed, bug 1238844 and bug 1186346 can be backed out.

Comment 10

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/81ab1a659071fb4b99852b1c611b341557f21ca4
Bug 1250871 - only send messages to child scripts when the docShell is still around. r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81ab1a659071