1259572 - Lots of crashes that look like they might be exploitable under nsSubDocumentFrame::DestroyFrom calling GetDocumentFromView

Status: RESOLVED WORKSFORME

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Boris Zbarsky [:bzbarsky]]

jesup asked me to file this, based on this crash-stats bit: https://crash-stats.mozilla.com/signature/?product=Firefox&address=~e5e5&signature=GetDocumentFromView&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports

Looks like we end up crashing on this bit:

  nsIFrame* f = aView->GetFrame();

Is it possible that in DestroyFrom mInnerView->GetFirstChild() was a garbage pointer?

Comment 1

[Daniel Veditz [:dveditz]]

possibly sec-critical if we find a reliable reproducer, starting with sec-high for now.

Comment 2

[Timothy Nikkel (:tnikkel)]

Patch in bug 1260531 might help with this.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

tnikkel is better placed to help with this than I.

Comment 4

[Timothy Nikkel (:tnikkel)]

This seems likely related to bug 1261175. Either way, the work that happens for that bug is likely to help here, so I'll post updates to that bug.

Comment 5

[Timothy Nikkel (:tnikkel)]

A decent chance bug 1265577 fixes this.

Comment 6

[Andrew McCreight [:mccr8]]

I see one crash on 50 in the last 7 days:
  https://crash-stats.mozilla.com/report/index/7c19fcd8-778b-4fec-99f2-0e42f2160708
...but none on 49. There are still a good number on 48.

Comment 7

[Andrew McCreight [:mccr8]]

There are still a good number of similar looking crashes in 50, and a handful in 51. I don't know if there's some release only issue, or if we fixed it somewhere.

Comment 8

[Frederik Braun [:freddy]]

Timothy, it seems you had an idea as to what bugs might or might not fix this. Care to take a look again? :)

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Current crashrate across all versions is ~50/day, so not rare

Comment 11

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

tnikkel - obviously the bug you referenced didn't fix this... can you have another look?  thanks

Comment 13

[Timothy Nikkel (:tnikkel)]

Yes, I wrote a prototype patch for what I wanted to do recently. Turning it into something landable requires thinking through a lot of complex interactions which I haven't had time to do.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Given this is somewhere between sec-high, and sec-critical, let's put this on the radar to get fixed soon (preferably 55 and uplift to 54?).  I'll fix-optional it for 53.  Crash-stats shows that this goes back to around Fx 24 or earlier.

Comment 15

[Stephanie Ouillon [:arroway] (needinfo me)]

Timothy, did you have a chance to work on this bug?

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: wip betterdetachedviews

I haven't had a chance to touch the prototype patch at all. Besides cleaning up this patch here is what needs to be done:

Besides in this patch, there are three other places in the code that touch root views that might conflict with this patch. nsDocShell::RestoreFromHistory plucks a view from its current location and places a different view in its place. nsDocumentViewer::MakeWindow creates a new root view and inserts it. nsDocumentViewer::Destory plucks a root view from the hierarchy and stashes it in the bf cache.

We need to check the following scenarios:
1) subdocumentframe stores one detached view
2) subdocumentframe stores one detached view, that view gets destroyed
3) subdocumentframe stores two or more detached views
4) subdocumentframe stores two or more detached views, they all get destroyed
5) subdocumentframe stores two or more detached views, the first one gets destroyed
6) subdocumentframe stores two or more detached views, the second one gets destroyed
and then each one of the above six followed by one (or more) of the three places from the previous paragraph, and then nsSubDocumentFrame::Init or nsHideViewer::Run runs.


It's not easy because I don't know how to trigger these conditions, but I think it is precisely these conditions happening which are the cause of this bug. If we don't get the interactions right then at best we don't fix the bug, at worst we make it worse.

Comment 17

[Frederik Braun [:freddy]]

(In reply to Timothy Nikkel (:tnikkel) from comment #16)
> It's not easy because I don't know how to trigger these conditions, but I
> think it is precisely these conditions happening which are the cause of this
> bug. If we don't get the interactions right then at best we don't fix the
> bug, at worst we make it worse.

Timothy, would someone else who works on that code be a helpful guide here?
If not, do you think we should wade through crash report URLs and try to find something reproducible?

Comment 18

[Daniel Veditz [:dveditz]]

Crashes in the signature and the query in comment 0 are only affecting old versions. Seems to have been fixed for all currently supported versions of firefox (ESR-52 and later)