1573823 - nsTypeAheadFind can hold on to a shell pointing to a document which already has another shell, somehow

Status: RESOLVED DUPLICATE of bug 1667209

(Core :: Find Backend, defect, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

We handle it safely (by not flushing), but it's another thing caught by bug 1530177.

See https://crash-stats.mozilla.org/signature/?version=70.0a1&signature=mozilla%3A%3APresShell%3A%3ADoFlushPendingNotifications&date=%3E%3D2019-08-06T19%3A29%3A00.000Z&date=%3C2019-08-13T19%3A29%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_columns=moz_crash_reason&_sort=-date&page=1#reports

All crashes from this assertion failing.

It seems we use it to go across iframes... it's not clear how should the code be changed but I can try to take a look.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So nsTypeAheadFind has mPresShell as a weakptr. So if it grabs the presshell and then the document gets a new one (e.g. via reframing of the <iframe>) but the old one hasn't been destroyed yet, we could end up working with it.

We could change nsTypeAheadFind::GetPresShell to do a sanity check and return null if that fails, I guess (like it already does when checking for a prescontext, etc). But fundamentally it seems like it would be slightly better if we held a weakptr to a document and then got a presshell from that. Or even simpler, just removed the mPresShell cache altogether (modulo maybe the use in nsTypeAheadFind::GetSearchContainers that compares it to the selection's presshell) and just reget it from the docshell as needed, which is what we do anyway if GetPresShell() returns null.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

So nsTypeAheadFind has mPresShell as a weakptr. So if it grabs the presshell and then the document gets a new one (e.g. via reframing of the <iframe>) but the old one hasn't been destroyed yet, we could end up working with it.

We could change nsTypeAheadFind::GetPresShell to do a sanity check and return null if that fails, I guess (like it already does when checking for a prescontext, etc). But fundamentally it seems like it would be slightly better if we held a weakptr to a document and then got a presshell from that. Or even simpler, just removed the mPresShell cache altogether (modulo maybe the use in nsTypeAheadFind::GetSearchContainers that compares it to the selection's presshell) and just reget it from the docshell as needed, which is what we do anyway if GetPresShell() returns null.

The issue as far as I understand it is that mDocShell points to the top-level document, while mPresShell may point to that or a subdocument.

But yeah keeping the document pointer as a weak ptr would work I suspect.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, because we can change mPresShell in the GetSearchContainers callers. OK. The other thing we could do is hold a weakref to the docshell that we are currently targeting, since that's where that presshell came from to start with.

Comment 4

[BugBot [:suhaib / :marco]]

The priority flag is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Mike de Boer [:mikedeboer]]

Emilio, does there remain anything left actionable in this bug? Please also note that bug 1553384 landed on autoland just now, which may include changes in the area you're looking at here.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

I think there is, yeah. https://searchfox.org/mozilla-central/search?q=nsTypeAheadFind%3A%3AmPresShell&case=false&regexp=false&path=

I don't know if that stuff is less reachable these days because of that bug though.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

I fixed this in bug 1667209.