1265577 - don't rely on the views we stash when we destroy a subdocument frame to stick around

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Timothy Nikkel (:tnikkel)]

Nothing forces them to stick around. The only thing that makes it true most of the time is we clear the stashed views on a script runner, limiting the total changes that can happen while we have them stored. But nothing prevents those changes from including destroying the stored views.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I wasn't sure if it was better to call frameloader->Hide() in nsSubDocumentFrame::DestroyFrom in the case that we aren't trying to stash the previous root frame, or to leave it to nsHideViewer to do that.

Comment 2

[Timothy Nikkel (:tnikkel)]

This could cause us to use a deleted view. We poison views when we delete them, but I don't think we hold that memory in an arena like frames, so it could be re-used right away.

Comment 3

[Tyson Smith [:tsmith]]

After a chat with :tnikkel it sounds like this patch could fix bug 1261175 and bug 1259572

Comment 4

[Tyson Smith [:tsmith]]

Marking sec-critical assuming this fixes bug 1259572 and because of the number of crash stats in bug 1259572 (it is likely reproducible).

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8742549 [details] [diff] [review]
patch

>layout/generic/nsSubDocumentFrame.cpp
>+    if (oldContainerDoc) {

The old code doesn't have this check, so it falls through to
"frameloader->Hide()" AFAICT (since OwnerDoc() is never null).
The new code does nothing in this case.
Why change the behavior for the null oldContainerDoc case?
Are you assuming 'detachedFrame' must be null in this case anyway?
Can we add an else-branch that MOZ_ASSERTs if so?
(I think it should hold)

>     nsView* detachedViews = ::BeginSwapDocShellsForViews(mInnerView->GetFirstChild());
>-    frameloader->SetDetachedSubdocView(detachedViews, mContent->OwnerDoc());
>+    bool tryToStashSubDoc =
>+      detachedViews && detachedViews->GetFrame() && !detachedViews->GetNextSibling();

The "!detachedViews->GetNextSibling()" condition is new here.
Why do we check that exactly?
Is the sibling view associated with the mPreviousViewer perhaps?
If so, could we simply destroy that viewer instead, so that we
can stash the frame here?

Also, in the !detachedViews->GetFrame() case, isn't there a perf loss
because we will recreate the sub doc pres shell/context when we reframe
an iframe that has no root frame yet (and this seems to happen more than
I thought - during startup for example).
Perhaps it's negligible though.

Comment 6

[Timothy Nikkel (:tnikkel)]

Thanks for your careful review of this hairy code!

(In reply to Mats Palmgren (:mats) from comment #5)
> >layout/generic/nsSubDocumentFrame.cpp
> >+    if (oldContainerDoc) {
> 
> The old code doesn't have this check, so it falls through to
> "frameloader->Hide()" AFAICT (since OwnerDoc() is never null).
> The new code does nothing in this case.
> Why change the behavior for the null oldContainerDoc case?

The old code used the stored views being null as a sentinel as to weather we try to use those stashed views, or call Hide on the frameloader. The new code uses a weak frame instead of a view. So there are two cases where we get a null frame: 1) we never stored a frame 2) we stored a frame but it was destroyed. In 1) we do not want to call Hide. For 2) we want to call Hide. So we can't use the presence or not of the stored frame to decide anymore. So instead I use the stored container doc check.

> Are you assuming 'detachedFrame' must be null in this case anyway?
> Can we add an else-branch that MOZ_ASSERTs if so?
> (I think it should hold)

Yes, I believe OwnerDoc can never return null. I can assert.

> >     nsView* detachedViews = ::BeginSwapDocShellsForViews(mInnerView->GetFirstChild());
> >-    frameloader->SetDetachedSubdocView(detachedViews, mContent->OwnerDoc());
> >+    bool tryToStashSubDoc =
> >+      detachedViews && detachedViews->GetFrame() && !detachedViews->GetNextSibling();
> 
> The "!detachedViews->GetNextSibling()" condition is new here.
> Why do we check that exactly?
> Is the sibling view associated with the mPreviousViewer perhaps?
> If so, could we simply destroy that viewer instead, so that we
> can stash the frame here?

Yes, good catch. I wanted to make this change in a separate patch but it turned out to require more work.

We have more than one view/frame here during document navigation; where there is a non-zero time with two "active" documents. This seemed like a bad time to do these kinds of acrobatics. The bad things that could happen though seem like they would be solved by storing weak frames. So I could create a patch that stores multiple weak frames and tries to restore them. It creates some more "interesting" questions though like what to do if one of two weak frames are destroyed? Call Hide? Just restore the one frame?

It seems more risky to introduce destroying the previous viewer, so I'd like to avoid that if possible (or proven to be the less risky option :) ).

> Also, in the !detachedViews->GetFrame() case, isn't there a perf loss
> because we will recreate the sub doc pres shell/context when we reframe
> an iframe that has no root frame yet (and this seems to happen more than
> I thought - during startup for example).
> Perhaps it's negligible though.

Only way I can think of to avoid that is to re-introduce weak views and store weak views. Worth it?

Comment 7

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> > The "!detachedViews->GetNextSibling()" condition is new here.
> [...]
> It creates some more "interesting" questions though like what to do if one
> of two weak frames are destroyed? Call Hide? Just restore the one frame?

Why can't we just drop that "!detachedViews->GetNextSibling()" check?
If one of these old navigations is destroyed it will destroy its frame
tree and its root view will be unhooked from the detachedFrame's view,
just like it's currently unhooked (as a sibling) from it.
The bug here is just that the (leading) stored detached view might
become stale, right?

Passing detachedFrame->GetView() to InsertViewsInReverseOrder should work
the same as it currently does if that view happens to have siblings.

Or do you have reason to believe the view sibling list is screwed up too?

> > Also, in the !detachedViews->GetFrame() case, isn't there a perf loss
> [...]
> Only way I can think of to avoid that is to re-introduce weak views and
> store weak views. Worth it?

Probably not.  Let's not worry about it for now.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #7)
> (In reply to Timothy Nikkel (:tnikkel) from comment #6)
> > > The "!detachedViews->GetNextSibling()" condition is new here.
> > [...]
> > It creates some more "interesting" questions though like what to do if one
> > of two weak frames are destroyed? Call Hide? Just restore the one frame?
> 
> Why can't we just drop that "!detachedViews->GetNextSibling()" check?
> If one of these old navigations is destroyed it will destroy its frame
> tree and its root view will be unhooked from the detachedFrame's view,
> just like it's currently unhooked (as a sibling) from it.
> The bug here is just that the (leading) stored detached view might
> become stale, right?
> 
> Passing detachedFrame->GetView() to InsertViewsInReverseOrder should work
> the same as it currently does if that view happens to have siblings.

Oh, I see what you are saying now. You are saying to just store the (weak) frame of the leading view, and use that to insert the leading view and any trailing siblings. The problem I see with that is that the leading weak frame can die, but one of the trailing views can still be alive, and we won't have any way to get a pointer to the trailing view to insert back into the tree.

The way I was thinking of allowing more than one view was to store multiple weak frames on the frame loader (in an array or similar). That would avoid that problem.

Comment 9

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> but one of the trailing views can still be alive

Can that actually happen?  Don't we always destroy the mPreviousViewer
chain in that case?  So if the leading view goes away so does all
its siblings.

> and we
> won't have any way to get a pointer to the trailing view to insert back into
> the tree.

If so, is that a bad thing?  Sooner or later these DocumentViewers will be
destroyed and associated frames/views destroyed, no?

> The way I was thinking of allowing more than one view was to store multiple
> weak frames on the frame loader (in an array or similar). That would avoid
> that problem.

Sure, an array of weak frames sounds fine in the long run to make this
independent of the view tree.  I think it's probably fine to just ignore
any null pointers in that case because that's how we currently handle it
(i.e. removed views have unhooked themselves so we won't even notice).

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #9)
> > and we
> > won't have any way to get a pointer to the trailing view to insert back into
> > the tree.
> 
> If so, is that a bad thing?  Sooner or later these DocumentViewers will be
> destroyed and associated frames/views destroyed, no?

The trailing view might be the view we actually want to display though. Actually, I think this will work fine. If the first weak frame is destroyed we'll call Hide and then Show will recreate any trailing views that should be there.


I think this is the approach I will take then.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8742549 [details] [diff] [review]
patch

Ok, thanks for the explanations.  r=mats with an added MOZ_ASSERT about
the frame being null when oldContainerDoc is, as suggested above.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: interdiff

interdiff from v1 to make sure we are on the same page.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8743558 [details] [diff] [review]
patch v2

Either one is fine, but I think I slightly prefer this version since it's close
to what we're currently doing for sibling views so it seems less risky.

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8743558 [details] [diff] [review]
patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
this isn't the easiest code to understand, but if an attacker groks the code they would know what general area to look in. I tried to reproduce this problem and I failed. But who knows what a determined attacker with lots of time and a fuzzer could do.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I'll remove the checkin comment before landing. So the answer is no.

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?
bug 775965 (it's on all supported branches)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
easy to make

How likely is this patch to cause regressions; how much testing does it need?
I think it's low risk

Comment 16

[Al Billings [:abillings - ex-MoCo]]

This has sec-approval to go in on May 10, two weeks into the release cycle (We're shipping Firefox 46 next week).

We'll want to get Aurora, Beta, and ESR45 patches, I expect. You may wish to prepare and nominate them now in order for them to go in following this landing on trunk.

Comment 17

[Timothy Nikkel (:tnikkel)]

Can we allow some bake time for this before uplifting? That would make me more comfortable.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Sure, we can do that normally. How long were you thinking? I can see a week easily. If you want more, maybe we should move up the checkin (with its associated risks) to May 3.

Comment 19

[Timothy Nikkel (:tnikkel)]

I think that would be a good idea.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking so that we remember to land this around May 10th and uplift it.

Comment 21

[Timothy Nikkel (:tnikkel)]

May 3 hopefully!

Comment 22

[Timothy Nikkel (:tnikkel)]

Can I land this?

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Yes, land it.

Comment 24

[Carsten Book [:Tomcat]]

the v2 patch has conflicts:
applying 1265577.patch
patching file layout/generic/nsSubDocumentFrame.cpp
Hunk #4 FAILED at 982
1 out of 4 hunks FAILED -- saving rejects to file layout/generic/nsSubDocumentFrame.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1265577.patch

does this need to be checkedin with the interdiff ?

Comment 25

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb3ce095a70f5a450dc321df946e0054be2bb79

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6eb3ce095a70

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Tim, could you please nominate patches for uplift to Beta47, Aurora48, ESR45? Thanks!

Comment 28

[Timothy Nikkel (:tnikkel)]

Attachment: patch for beta

Aurora should be able to the patch from central. Beta had some conflicts.

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: patch for esr45

This touches a lot of the same lines as bug 1261230. Rather than re-write the patch we should just uplift bug 1261230, I think that is the least risky option. I will nominate bug 1261230 for uplift.

Comment 30

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8743558 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 775965
[User impact if declined]: potentially security crashes
[Describe test coverage new/current, TreeHerder]: not specifically for this problem because I haven't been successful reproducing it.
[Risks and why]: this is the least risky way of fixing this problem.
[String/UUID change made/needed]: none

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8751039 [details] [diff] [review]
patch for beta

Approval Request Comment
[Feature/regressing bug #]: bug 775965
[User impact if declined]: potentially security crashes
[Describe test coverage new/current, TreeHerder]: not specifically for this problem because I haven't been successful reproducing it.
[Risks and why]: this is the least risky way of fixing this problem.
[String/UUID change made/needed]: none

Comment 32

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8751040 [details] [diff] [review]
patch for esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-crit
Fix Landed on Version: 49
Risk to taking this patch (and alternatives if risky): this is the least risky way of fixing this problem.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8743558 [details] [diff] [review]
patch v2

Sec-crit, Aurora48+

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/efacf499e4b5

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d7bbfed978a2e0aceff2bbdd8c8d72f5137081d

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8751040 [details] [diff] [review]
patch for esr45

Sec-critical, it will ship with 47, let's get this into esr45.

Comment 37

[Carsten Book [:Tomcat]]

  https://hg.mozilla.org/releases/mozilla-esr45/rev/ee870911fabb