292962 - Need to sort out policy on DOM access in cached documents

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Specifically, need to figure out what happens on DOM method calls, on DOM
mutations, etc, etc.

Note that the current fastback/bfcache code assumes the DOM won't mutate, and
probably crashes if it does mutate in certain ways (it's holding weak refs to
docshells that are only kept alive by the DOM in question and which can lose the
ref from the DOM if the frame elements are removed).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I have a patch in the works for this, needs a bit more testing.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

I had to add the mDocument member because some callsites first calls Destroy()
on the contentviewer before calling SetContentViewer(nsnull) on the SHEntry. In

that case we wouldn't be able to get to the document to remove ourselvs as
observer causing the document to have a dangling pointer once the SHEntry goes
away.

The PLEvent fixes two problems. First of all we don't want to risk dropping the

last reference to the contentviewer during mutation causing the contentviewer
and document to die. Second, calling Destroy on the viewer during mutation
seemed to remove some other documentobservers (probably the presshell). The
notification code can't deal with observers other then the current one being
removed during notification.

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 187179 [details] [diff] [review]
patch to fix

requesting approval

Comment 4

[chris hofmann]

Comment on attachment 187179 [details] [diff] [review]
patch to fix

a=chofmann

Comment 5

[Brian Ryner (not reading)]

checked in

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase file 1

Just attaching the testcases I used to test this so that they don't get lost

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase file 2

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sorry, that patch doesn't fix the bug as filed.  It fixes the "DOM mutations"
case, but doesn't address "DOM method calls", which is just as big an issue (and
can have security implications unless we've audited every single DOM method to
ensure that things are OK).

If there's a followup bug filed on the remainder of the work here, please feel
free to reresolve this one, but let me know what the new bug is.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

what DOM calls are you reffering to?

I can think of two classes of calls:

1. Pure access methods like getElementsByTagName() and getElementById()
2. Navigational methods like link.click() and form.submit()

1 I don't think is a problem and should be allowed. 2 I thought we had a
separate bug on though I can't find it right now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

You missed at least:

3.  Other calls that use the docshell (not explicitly for navigational
    purposes).  Are these currently enabled in bfcached documents?
4.  Calls that use the presentation (computed style, offset*, scrolling to a 
    particular element, etc, etc).
5.  Event dispatch (really not sure what the behavior should be here).

I can't think of obvious things in category 3, but I wouldn't bet there aren't
any.  For example, some of our security checks require a docshell at the moment.

We need to decide on what the behavior for these should be, and specifically
whether it should differ between documents that are in bfcache and documents
that aren't but have been navigated away from (and if so how).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Looking at the patch in more detail, I'm also a little worried by script that
mutates the old document and then calls history.back().  This will make part of
the back() happen before our PLEvent fires (and part after, if I recall the
control flow for bfcache correctly).  Which means that we would partially
restore a presentation, then get it wiped out, then proceed to do some more
restoring on the same presentation...

Comment 12

[Brian Ryner (not reading)]

Pulling into b4 for investigation.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm on this one, i hope to have some code ready this weekend.

Comment 14

[Chris Beard]

Any update here?  Triaging for 1.5.

Comment 15

[Mike Schroepfer]

Sicking are you still on this?

Comment 16

[Asa Dotzler [:asa]]

Johnny and BZ, it seems Jonas is away. Any chance you guys can tackle this problem?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Not for at least a few weeks in my case.  I'm swamped with existing 1.8b5
blockers (not bfcache dependent) and teaching.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, i'm out of my hole. Back to me.

Comment 19

[Asa Dotzler [:asa]]

Jonas, any progress here? Time is short for 1.5.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I've got a mostly working patch, but it needs a bit more testing. Will attach
tomorrow.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix remaining issues

This should take care of the remaining issues I hope. I've beaten this hard and
it seems to behave properly.

While the document is in the bf cache it is unaware of its presshells which
seems to make it behave exactly as a document that doesn't have any
presentation at all.

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 198313 [details] [diff] [review]
Fix remaining issues

>--- content/base/src/nsDocument.cpp	24 Sep 2005 18:43:08 -0000	3.585
>+++ content/base/src/nsDocument.cpp	3 Oct 2005 15:06:52 -0000
>@@ -1520,12 +1523,43 @@ nsDocument::GetNumberOfShells() const
> nsIPresShell *
> nsDocument::GetShellAt(PRUint32 aIndex) const
> {
>   return (nsIPresShell*)mPresShells.SafeElementAt(aIndex);
> }
> 
>+nsresult
>+nsDocument::SetShellsHidden(PRBool aHide)
>+{

The two cases in this function do exactly the same thing, just with the
variables reversed.  How about factoring it out:

SwapArrays(nsSmallVoidArray *from, nsSmallVoidArray *to);

On second thought, wouldn't it be a lot easier to just keep a single array,
with a boolean that says whether the shells are hidden, and a getter that
returns null if it's set?  Or were you worried about efficiency of that
approach?


>--- docshell/base/nsDocShell.cpp	23 Sep 2005 18:16:38 -0000	1.745
>+++ docshell/base/nsDocShell.cpp	3 Oct 2005 15:06:59 -0000
>@@ -5402,19 +5402,19 @@ nsDocShell::RestoreFromHistory()
>     // Grab the window state up here so we can pass it to Open.
>     nsCOMPtr<nsISupports> windowState;
>     mLSHE->GetWindowState(getter_AddRefs(windowState));
> 
>     mLSHE->SetWindowState(nsnull);
> 
>-    // Reattach to the window object.
>-    rv = mContentViewer->Open(windowState);
>-
>-    // Now remove it from the cached presentation.
>+    // Remove it from the cached presentation.
>     mLSHE->SetContentViewer(nsnull);
>     mEODForCurrentDocument = PR_FALSE;
> 
>+    // ...and reattach to the window object.
>+    rv = mContentViewer->Open(windowState);
>+

Did one of the changes to SHEntry make this change necessary?


>--- docshell/shistory/src/nsSHEntry.cpp	23 Sep 2005 18:16:39 -0000	1.45
>+++ docshell/shistory/src/nsSHEntry.cpp	3 Oct 2005 15:06:59 -0000
>-
>-  // Ensure that we don't post more then one PLEvent
>-  RemoveDocumentObserver();
>+  else {
>+    // Drop presentation. Also ensures that we don't post more then one
>+    // PLEvent. Only do this if we succeed to post the event since otherwise
>+    // the document could be torn down mid mutation causing crashes.
>+    DropPresentationState();
>+  }
> }

This had me confused for awhile, until I realized that DropPresentationEvent no
longer calls DropPresentationState.  Could you rename it to something a little
clearer, like DestroyViewerEvent?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> On second thought, wouldn't it be a lot easier to just keep a single array,
> with a boolean that says whether the shells are hidden, and a getter that
> returns null if it's set?  Or were you worried about efficiency of that
> approach?

I pondered this approach too. There are a lot of places where we do use the
mPresShells member directly. Originally I wanted to avoid the virtual calls of
GetShellAt and GetNumberOfShells, but on second thought I doubt that it'll matter.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix remaining issues v2

Fixes comments by bryner. I also made the event hold a reference to the
document just so that the document doesn't die if contentviewer releases the
reference it is holding for some reason or the other.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase file 1

Updated test files

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase file 2

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase file 3

Comment 28

[Scott MacGregor]

Jonas, what are the end user ramifications with this patch vs. without it. What
things would the user see? We are trying to figure out if we'll approve this
change right before the deadline. 

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There is really little that changes for the end user with this patch. The only
thing should be that the user can no longer get to style information while the
document is in the bfcache (i.e. the document will behave as if there was no
bfcache).

The patch might have fixed a crash (didn't test if it crashed before, but it
doesn't after), though that crash is fairly unlikly to be hit. It is the 'mutate
and back' scenario described in comment 11.

The patch should be farily safe. Mostly because I've pounded on it pretty hard
and there were no surprices.

Comment 30

[Brian Ryner (not reading)]

Comment on attachment 198352 [details] [diff] [review]
Fix remaining issues v2

>--- docshell/shistory/src/nsSHEntry.cpp	23 Sep 2005 18:16:39 -0000	1.45
>+++ docshell/shistory/src/nsSHEntry.cpp	3 Oct 2005 21:02:24 -0000
>@@ -111,15 +111,18 @@ ClearParentPtr(nsISHEntry* aEntry, void*
> nsSHEntry::~nsSHEntry()
> {
>   // Since we never really remove kids from SHEntrys, we need to null
>   // out the mParent pointers on all our kids.
>   mChildren.EnumerateForwards(ClearParentPtr, nsnull);
>   mChildren.Clear();
>-  RemoveDocumentObserver();
>-  if (mContentViewer)
>-    mContentViewer->Destroy();
>+
>+  nsCOMPtr<nsIContentViewer> viewer = mContentViewer;
>+  DropPresentationState();

I still don't understand the need to call DropPresentatinoState from the dtor.


>+    // Drop presentation. Also ensures that we don't post more then one
>+    // PLEvent. Only do this if we succeed to post the event since otherwise

"succeeding in posting the event" would sound better to me.

In nsDocument, what do you think about making inline, non-virtual versions of
GetShellAt and GetNumberOfShells?  They'd need to be named differently.

Comment 31

[Brian Ryner (not reading)]

(In reply to comment #30)
> "succeeding in posting the event" would sound better to me.

succeeded, rather.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Maybe what we could do for the branch is to just take the part of this patch
that changes the PLEvent. This is the part that fixes actual hazards, the rest
is more a correctness issue.

Btw, I still havn't made any changes for the 3 issue in comment 10. I think the
current sitation is fine actually. A document in the bfcache will behave like a
documents did before we had the bfcache as neither can get to the docshell. Not
sure what security checks uses the docshell, but they better deny access if they
can't get to it, otherwise they need to be fixed either way.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> I still don't understand the need to call DropPresentatinoState from the dtor.

This makes us unhide the presshells in the document so that the document has the
same state as it usually has during teardown. I'm not sure if this is needed or
not, but it seemed safest.

> >+    // Drop presentation. Also ensures that we don't post more then one
> >+    // PLEvent. Only do this if we succeed to post the event since otherwise
> 
> "succeeding in posting the event" would sound better to me.
> 
> In nsDocument, what do you think about making inline, non-virtual versions of
> GetShellAt and GetNumberOfShells?  They'd need to be named differently.

I thought about that too, but it feels like it mostly gets messy with the naming
and such. I doubt that there are any performance issues with this since these
are functions that are called just a handfull of times during the lifetime of a
document.

We could do is to place mPresShells and mShellsAreHidden in nsIDocument and
inline GetShellAt and GetNumberOfShells... Don't have a strong oppinion either way.

Comment 34

[Scott MacGregor]

(In reply to comment #32)
> Maybe what we could do for the branch is to just take the part of this patch
> that changes the PLEvent. This is the part that fixes actual hazards, the rest
> is more a correctness issue.

I like that idea, could you post a branch patch that did just that for us to
evaluate? Thanks!

Comment 35

[Brian Ryner (not reading)]

Comment on attachment 198352 [details] [diff] [review]
Fix remaining issues v2

You're right, none of those call sites seem very performance-critical.	We can
go with this patch if you'd like.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: branch patch

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: branch patch with better wording

Fixed bryners wording comment. Attaching a new patch since i gotta head off to
bed. Blake has offered to land the patch if it gets approved.

Comment 38

[Scott MacGregor]

Comment on attachment 198390 [details] [diff] [review]
branch patch with better wording

Jonas, thanks again for making the branch only patch for us. And thanks bryner
for double checking that patch.

I'm approving this per earlier discussions with folks.

Blake, if you can't check this in tonight, let me know and I will.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in on MOZILLA_1_8_BRANCH.

Comment 40

[Scott MacGregor]

This bug may have caused a crash with yahoo.com. See Bug 311269 for more details.

Comment 41

[Scott MacGregor]

clearing the fixed 1.8 flag since we backed this out. sicking is investigating
311269 too.

Comment 42

[Frank Yan (:fryn)]

(In reply to comment #41)
> clearing the fixed 1.8 flag since we backed this out. sicking is investigating
> 311269 too.

311269 has been fixed on 1.8.

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 198352 [details] [diff] [review]
Fix remaining issues v2

Landed this and the patch from bug 311269 on the trunk

Comment 44

[Asa Dotzler [:asa]]

Comment on attachment 198390 [details] [diff] [review]
branch patch with better wording

ok. we need to get this in asap.

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed the branch-safe parts of this on the branch and the remaining parts has
been on the trunk for a while, so marking fixed.

We still don't hide the presshells on bfcached docs on the branch, but i think
it's safer to let that be as is at this point.

Comment 46

[Boris Zbarsky [:bzbarsky]]

So what do we do on branch when the DOM mutates?

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As far as mutations go we do the same on trunk and branch: immediately drop the
presentation from the shentry and then release the contentviewer and document on
an event.

The difference is that on trunk if you access style information on a bfdocument
it will behave as a data-only document, whereas on the branch you get the same
style information as when the document was last shown.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  I guess I can live with that.  ;)