Closed Bug 1250871 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 47
Iteration:
47.3 - Mar 7
Tracking Status
e10s - ---
firefox47 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

Attachments

(1 file)

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.
Is this about e10s or non-e10s or both?
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?
(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.
Blocks: 1126089
(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.
Blocks: loop-e10s
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.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8723778 - Flags: review?(bugs)
Iteration: --- → 47.3 - Mar 7
Points: --- → 2
After this is fixed, bug 1238844 and bug 1186346 can be backed out.
Attachment #8723778 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/81ab1a659071
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.