301516 - fastback breaks history.back()

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Brian Ryner (not reading)]

Steps to reproduce:

1. Load the attached testcase
2. Click "Go Back"
3. Click the Forward button

Result:

The current page continues to be shown, even though the location bar updates.

Expected result:

The testcase should be shown again.

Comment 1

[Brian Ryner (not reading)]

*** Bug 301517 has been marked as a duplicate of this bug. ***

Comment 2

[Brian Ryner (not reading)]

Attachment: testcase

Comment 3

[Brian Ryner (not reading)]

Note, the problem only seems to occur if I save the testcase locally.

Comment 4

[Ria Klaassen (not reading all bugmail)]

The testcase does not seem to work on secure pages like bugzilla. :)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

After step 2 in your steps to reproduce, I see:

Error: uncaught exception: Permission denied to set property Window.navigator

in the javascript console, fwiw.

Comment 6

[Brian Ryner (not reading)]

Yeah, I see a failure to copy one of the properties from the page we're going
back to as well.  In my case it's just a function defined in the page. It seems
to be failing inside of XPC_WN_Helper_AddProperty, called from
js_DefineNativeProperty.  I haven't quite worked out what's failing, because of
all the macro fu in that function.

Comment 7

[Brian Ryner (not reading)]

It looks like it's failing the security check... I have a feeling what's
happening is that it sees the JS from the testcase page on the stack and thinks
that's what is trying to set the property on the other site's window.

Comment 8

[Brendan Eich [:brendan]]

jst can probably add value here; cc'ing bz too, since he's been into xpconnect
for the XPCNativeWrapper automation work in 1.8.

/be

Comment 9

[Brendan Eich [:brendan]]

bryner: so if nsGlobalWindow::RestoreWindowState is called with the top cx on
the xpconnect thread context stack being something other than the cx used in
that method (which is the nsGlobalWindow's mContext's native context), then
yeah, you will hit security hassles.  So temporarily push that cx on the
threadcontextstack. jst prolly has a C++ auto-storage-class helper lying around.

/be

Comment 10

[Brian Ryner (not reading)]

Hm, I tried using the JSContextStack to push the window's JSContext before
copying the properties back from the property-store object.  It doesn't seem to
help though.

+  cxStack->Push(cx);
   nsresult rv = CopyJSPropertyArray(cx, aSource, aDest, props);
+  JSContext *dummy;
+  cxStack->Pop(&dummy);

I still get an error, "Permission denied to set property Window.sf" (sf is a
function defined on http://google.com/).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushing a null cx may work better, actually, if we're sure that no JS will run
while we have it pushed...  See
http://lxr.mozilla.org/seamonkey/source/layout/forms/nsTextControlFrame.cpp#3041
for an example.

That said, the AddProperty classinfo hook for window will flat-out throw for
some properties no matter what (eg for "location") unless
sDoSecurityCheckInAddProperty is false in classinfo.  So maybe what we really
want here is to have the window call something on nsWindowSH to set that to
false, then copy stuff, then set it back to true.  Again, if we're sure nothing
bad can happen in there...

Comment 12

[Brendan Eich [:brendan]]

Pushing cx is better than pushing null; sounds like you need an nsWindowSH
friend function or two to call too, to clear and set that static.  jst can weigh
in, but it should be easy to do -- dom/src/base files can be tight this way
without a bad modularity hazard resulting.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

If we do the classinfo thing (which I think we should), we should have no need
to push a context.

Comment 14

[Brendan Eich [:brendan]]

When split windows land, we can do away with copying altogether.

But, in the mean time, since setters do security checks, and since that static
does not control all setters' (or addProperty hooks') security checks, it seems
to me better to push and pop cx.  But I admit, I haven't yet identified a setter
that might check and declare "access denied".

/be

Comment 15

[Brian Ryner (not reading)]

Well, that static (sDoSecurityCheckInAddProperty) causes us to call into
ScriptSecurityManager, but I don't really understand why that security check
still fails after I've pushed the window's context.

Comment 16

[Brian Ryner (not reading)]

per discussion with jst, we realized that making this work this way would open a
security hole, since the window would contain all of the properties of the site
you're going back to after history.back() returns.

I think the way to solve this is to move the window state restoration (and
probably some other things) into the FinishRestore event processing instead of
doing them synchronously.

Comment 17

[Brian Ryner (not reading)]

Attachment: patch v1

Do the majority of RestorePresentation's work from a PLEvent.  Seems to fix
this bug, but I need to do much more testing on it.

Comment 18

[Darin Fisher]

Actually, bryner and I spoke about this patch some more and we decided that it
makes sense to move two other things into RestoreFromHistory (i.e., from a PLEvent):

> mEODForCurrentDocument = PR_FALSE;

and the call to FirePageHideNotification.

Comment 19

[Brian Ryner (not reading)]

Attachment: patch v2

Fixed the things Darin mentioned, plus:

- Do a second CanSavePresentation check in RestoreFromHistory, just to be sure,
and to more closely match the non-fastback code path.  Don't save this state on
the plevent anymore.
- Move things around in RestoreFromHistory to more closely match the
non-fastback code path.  Added comments to explain what part of the
non-fastback code each section corresponds to.

Comment 20

[Brian Ryner (not reading)]

Attachment: patch

Oops, fix the CanSavePresentation check to ignore the request we just added to
the loadgroup.

Note that this patch does not really handle the case where we fail for some
reason to restore the presentation on the plevent.  I'm still thinking about
the best way to handle that -- I think I basically want to clear out the
presentation on mLSHE and restart the load.  But all of the parameters needed
to do that will have to be cached on the plevent for that to work.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I'll take me a week or more to review this; I need to sort out the fastback code
that I never reviewed when it landed in the process of reviewing this...

Comment 22

[Darin Fisher]

Restarting the load from the PLEvent as you suggest seems reasonable given that
nsDocShell::Stop calls RevokeEvents.

Comment 23

[Darin Fisher]

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

>Index: nsDocShell.cpp

>+    event->mDocShell->RestoreFromHistory();
>+    // XXX need to fail over and do a normal load if something fails.

How about capturing a result code from this function call, and then
add a debug assertion to alert developers if this error case is being
hit.  That may help you to gather information about when that condition
may occur.


>+    nsresult rv = uiThreadQueue->PostEvent(evt);
>+    if (NS_FAILED(rv))
>+        PL_DestroyEvent(evt);
>+
>+    // The rest of the restore processing will happen on our PLEvent callback.
>+    *aRestored = PR_TRUE;
>+    return NS_OK;

hrm... in the PostEvent failure case, we set aRestored to true.
should we?

maybe this parameter should be named aRestoring instead?


>+    if (++gNumberOfDocumentsLoading == 1)
>+      PL_FavorPerformanceHint(PR_TRUE, NS_EVENT_STARVATION_DELAY_HINT);
>+
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mContentViewer->GetDOMDocument(getter_AddRefs(domDoc));
>+    nsCOMPtr<nsIDocument> document = do_QueryInterface(domDoc);
>+    if (document) {
>+        SetCurrentURI(document->GetDocumentURI(),

tweak indentation to be consistent


r=darin

Comment 24

[Brian Ryner (not reading)]

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

requesting approval. I discussed this patch with Boris, and he said it would be
ok if the patch landed with darin's review, then he can review later.

Comment 25

[Brian Ryner (not reading)]

checked in

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Looks reasonable, though I filed bug 306283 on what looks like an issue with
revoking the restore events....