Closed Bug 301516 Opened 17 years ago Closed 17 years ago

fastback breaks history.back()

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: bryner, Assigned: bryner)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
*** Bug 301517 has been marked as a duplicate of this bug. ***
Attached file testcase
Note, the problem only seems to occur if I save the testcase locally.
Target Milestone: --- → mozilla1.8beta4
The testcase does not seem to work on secure pages like bugzilla. :)
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.
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.
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.
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
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
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/).
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...
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
If we do the classinfo thing (which I think we should), we should have no need
to push a context.
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
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.
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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #190201 - Flags: review?(darin)
Attachment #190201 - Flags: review?(darin) → review+
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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #190201 - Attachment is obsolete: true
Attachment #190275 - Flags: review?(darin)
Attached patch patchSplinter Review
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.
Attachment #190275 - Attachment is obsolete: true
Attachment #190278 - Flags: superreview?(bzbarsky)
Attachment #190278 - Flags: review?(darin)
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...
Attachment #190275 - Flags: review?(darin)
Restarting the load from the PLEvent as you suggest seems reasonable given that
nsDocShell::Stop calls RevokeEvents.
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
Attachment #190278 - Flags: review?(darin) → review+
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.
Attachment #190278 - Flags: approval1.8b4?
Flags: blocking1.8b4+
Attachment #190278 - Flags: approval1.8b4? → approval1.8b4+
checked in
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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....
Attachment #190278 - Flags: superreview?(bzbarsky) → superreview+
Depends on: 360511
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in before you can comment on or make changes to this bug.