Closed Bug 534178 Opened 10 years ago Closed 9 years ago

When loading Plugin Check page, browser back does not work & unexpected same URL entries are generated in navigation history.

Categories

(Core :: DOM: Navigation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: alice0775, Assigned: smaug)

References

()

Details

(Keywords: regression)

Attachments

(4 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b6pre) Gecko/20091210 Firefox/3.5.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b6pre) Gecko/20091210 Firefox/3.5.5

Browser back des not work when I open URL.
The Back button is enabled but does not work.
And also, Select the navigation history in the pull down menu beside the Back-Forward button.


Reproducible: Always

Steps to Reproduce:
1.Start Namoroka with New Profile
2.Confirm some plugins such as Flash Player, Java etc, are installed.
3.Open URL
4.Click Back button or select the navigation history in the pull down menu beside the Back-Forward button.


Actual Results:  
The back button is ennabled. But it does not work.
And also, Select the navigation history in the pull down menu beside the Back-Forward button.it does not work.


Expected Results:  
The back-Foeward button and the pull-down menu should work properly.

This issue happens on the trunk build also.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091210 Minefield/3.7a1pre ID:20091210043057
Version: unspecified → 3.6 Branch
Summary: Browser back des not work when I open the URL → Browser back des not work when I open https://www.mozilla.com/en-US/plugincheck/
webNavigation may be broken something on Firefox3.6.x & Minefield3.7a2pre .
Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/5ab96ef362ae
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090122 Firefox/3.2a1pre ID:20090122033712


Broken:
http://hg.mozilla.org/mozilla-central/rev/e1962b8ae522
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Firefox/3.2a1pre ID:20090123033224

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5ab96ef362ae&tochange=e1962b8ae522
Keywords: regression
 This issue also happens on  page ( http://www.sony.jp/bravia/lineup/52v.html )
 Number of entries in the pulldown menu of Back-Foward button is more than 3.
 It should be 0.
 Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2pre) Gecko/20100209 Namoroka/3.6.2pre ID:20100209045227

 The issue does not happens Firefox3.5.8 , Google Chrome4.0.249.78, Safari4.0.4 and IE8.
Regress Bug 474349 -  [FIX]Should replace mOSHE when starting a wyciwyg load no matter what  

When I backed out change set of 6b8f3706a36f, the issue does not happen.
Blocks: 474349
Component: General → Document Navigation
Product: Firefox → Core
Version: 3.6 Branch → 1.9.2 Branch
Summary: Browser back des not work when I open https://www.mozilla.com/en-US/plugincheck/ → Browser back does not work, unexpected same URL entries are generated in navigation history.
QA Contact: general → docshell
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100211 Minefield/3.7a2pre {Build ID: 20100211085823}

Reproducible with trunk/Windows XP SP3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 1.9.2 Branch → Trunk
In addition to the Comment #4,
When I did a backout of Bug 47349(change set 6b8f3706a36f), the testcase of Bug 459443 still succeeds.

So, I think that we have to review Bug 47349 once again.
And I think Bug 47349 should be backed out.
It's possible, but I'd rather figure out why that patch caused an issue here instead.  This bug is near the top of my todo list, for what it's worth.
Sorry, I mistaken in Comment #6,
s/Bug 47349/Bug 474349/g
Duplicate of this bug: 550064
Summary: Browser back does not work, unexpected same URL entries are generated in navigation history. → When loading Plugin Check page, browser back does not work & unexpected same URL entries are generated in navigation history.
OS: Windows 7 → All
Hardware: x86 → All
It is also a problem on webpages using the plugin check badge.
(E.g. http://buhera.blog.hu/2010/05/11/plug_in_ellenorzest_mindenkinek)

I tried the following versions of Firefox, and the problem occurs in all of them:

Mozilla/5.0 (Windows; U; Windows NT 6.1; hu; rv:1.9.2.4) Gecko/20100503 Firefox/3.6.4

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100511 Minefield/3.7a5pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; hu; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

In these cases the drop-down list near the "back" button shows the plugin check webpage (or the webpage using the badge) several times.

In Opera the "back" button still doesn't work, but the page occurs only once in the drop-down list.

Opera/9.80 (Windows NT 6.1; U; hu) Presto/2.5.24 Version/10.53

It works normally in other web browsers:

Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; InfoPath.3)

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.1.249.1064 Safari/532.5
The author of the above site removed the badge because of this problem, there is an other site using the badge: http://www.readwriteweb.com/archives/are_your_web_browser_plugins_safe_this_page_tells_you.php
Here, this link makes duplicate navigation history.
http://www.sony.jp/walkman/lineup/neck_strap.html

as mentioned in comment#4
Regress Bug 474349 -  [FIX]Should replace mOSHE when starting a wyciwyg load no
matter what  .

When I backed out change set of 6b8f3706a36f, the issue does not happen.

I'd suggest to back out the change set 6b8f3706a36f since it was landed without
proper reviews and has regressions.
I don't know where you got your "without proper reviews" thing from; for the rest I've actually been debugging this today.  More info to come as it appears.
OK.  So the pluginscheck page in fact does document.open/write/close in a number of iframes after the main page has loaded, including an iframe that it adds and then removes.  That last is what prevents going back in history, but I think bug 462076 is supposed to address that.

Which just leaves the question of why those open/write/close calls didn't use to add session history entries and now do so.  The issue is that we pass mOSHE as aCloneRef to AddChildSHEntry.  Before, for document.write-generated documents this was always null, so we always had null aCloneRef, and hence did the AddChild thing instead of actually cloning the SH tree.

I think we should focus on reviewing and landing bug 462076, then see what the behavior looks like.  It'd be pretty easy to restore the old handling of document.open mOSHE for purposes of aCloneRef while still setting the mOSHE correctly.
Depends on: 462076
This testcase works on current trunk and 3.6, but is broken on 3.5, as expected.

Of course if the <script> after the iframe is removed it starts to work on 3.5, since then mOSHE ends up set by the about:blank load in the iframe.

That sort of racy behavior is really bad; we should avoid it.  The only question is what's web-compatible here.  In general, document.open() on a subframe after onload creates new SHentries in Gecko so that you can navigate to the state before the open() call.

I tested some other browsers.  Safari doesn't create new SHEntries for open() calls (so completely fails this testcase and all variations).  Opera does, and passes this testcase, but the plugincheck page is totally broken in Opera so I can't tell how it would behave there.  IE8 passes this testcase, but the plugincheck page isn't showing me any plug-ins in IE, so I can't tell what it would do with the plugincheck gunk.

In any case, this is only about whether we get "extra" entries in the session history.  The inability to go back is probably something for bug 462076 to fix.
So,
Do we back out of bug 474349 till bug 462076 is fixed?
No, why would we?  As I mentioned, that would regress other testcases (making back navigation completely impossible in them, unlike what we have now where it's still possible; worst-case via the dropdown menu).
Google Chrome and Safari fails the testcase Created an attachment (id=445517).
And I think the testcase does not exist in real world.
Yes, webkit also fails anything else involving document.open() and history navigation; see comment 15.  Such pages definitely exist in the real world; webkit's session history is just totally broken.  Just ask smaug.
In any case, the goal is to get bug 462076 fixed asap (this week, ideally).
Assignee: nobody → bzbarsky
blocking2.0: --- → beta4+
Target Milestone: --- → mozilla2.0b2
Priority: -- → P1
Target Milestone: mozilla2.0b2 → ---
Bumping, won't happen for beta4.
blocking2.0: beta4+ → beta5+
This is not critical for beta5.
blocking2.0: beta5+ → betaN+
OK, in IE what happens with the attached testcase if I take out the call to f() during load is that the first call to f() does NOT add a new history entry.  It does in Gecko, though.  I should fix that, but that won't help with the plugin check page; I see a bunch of open() calls on the same document in row there.... but the keys is this the docshell involved goes away after that.  Looking into what's going on with that.
What's going on is that there are 2 open() calls, then the frame is removed (and the shistory fixed up), then 2 more open() calls, then the frame is NOT removed.  So those two SHentries stick around.

Similarly, for the comment 12 url there are two open() calls and nothing is removed after that.
OK for comment 12, the iframe _is_ removed.  We remove the two child entries, but leave the two separate toplevel entries (should we?).  But when actually hitting back we skip over one of the toplevel entries and go right back to the previous page now, as a result of bug 462076.

Actually, that's what happens on the plugin check page too; not sure what I saw in comment 24.  We remove all the subframe entries for the various iframes when we remove the iframes from the DOM, but the toplevel ones are still present in the history menu.  They're just skipped over when navigating.  

smaug, any way we can remove the redundant toplevel shentries in that situation?
So in other words, "browser back does not work" is fixed here unless the plugin page is the first one you load (in which case there's nowhere to go back to, but the back button is enabled due to the spurious entries).
(In reply to comment #25)
>  but the toplevel ones are still present in
> the history menu.  They're just skipped over when navigating.  
> 
> smaug, any way we can remove the redundant toplevel shentries in that
> situation?

Yeah, this is a known problem. We could try to remove shentry (sub)trees if they
are identical, I think.
(In reply to comment #27) 
> Yeah, this is a known problem. We could try to remove shentry (sub)trees if
> they
> are identical, I think.

I could look at this next week.
The relevant method is nsSHistory::RemoveEntries.
Attached patch wip (obsolete) — Splinter Review
Tested using the plugins page.
I need to push this to tryserver.

And if anyone has better ideas for the OnLocationChange thingie.
Attached patch patch (obsolete) — Splinter Review
Fixed the mistake where mLength was decreased only when aIndex == mIndex.
Attachment #470439 - Attachment is obsolete: true
Attachment #471121 - Flags: review?(bzbarsky)
Comment on attachment 471121 [details] [diff] [review]
patch

Based on tryserver this causes a crash somewhere in reftest.
Unfortunately reftests don't work on my machine.
Trying another one...
Attachment #471121 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
Posting this to tryserver. May or may not fix the problem.
(reftest doesn't work on my machines at all.)
Attachment #471121 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Ok, this fixes the crash.
Attachment #471487 - Flags: review?(bzbarsky)
Assignee: bzbarsky → Olli.Pettay
Comment on attachment 471487 [details] [diff] [review]
patch

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>+nsDocShell::HistoryTransactionRemoved(PRInt32 aIndex)

>+    } else if (aIndex < mPreviousTransIndex) {
>+        mPreviousTransIndex = NS_MAX(-1, mPreviousTransIndex - 1);

Can aIndex be < -1?  If not, why is the NS_MAX with -1 needed?

>+    } else if (aIndex < mLoadedTransIndex) {
>+        mLoadedTransIndex = NS_MAX(0, mLoadedTransIndex - 1);
>+    }

Can aIndex be < 0?


> GetPrincipalDomain(nsIPrincipal* aPrincipal, nsACString& aDomain)
> {
>   aDomain.Truncate();
>-
>+                     

Undo that, please.

>@@ -9623,17 +9653,17 @@ nsDocShell::AddState(nsIVariant *aData, 
>-        FireOnLocationChange(this, nsnull, mCurrentURI);
>+        FireDummyOnLocationChange();

Why is this still needed?  Or is it just a nicer-looking cleanup kinda thing?

>+++ b/docshell/base/nsIDocShell.idl

Can we put the new member at the end?  Or better yet on nsIDocShell_MOZILLA_2_0_BRANCH at this point....

>+++ b/docshell/shistory/src/nsSHTransaction.cpp
> nsSHTransaction::SetNext(nsISHTransaction * aNext)
> {
>+  if (aNext) {
>    NS_ENSURE_SUCCESS(aNext->SetPrev(this), NS_ERROR_FAILURE);

Fix the indent here.

>+++ b/docshell/shistory/src/nsSHistory.cpp
>+PRBool IsSameTree(nsISHEntry* aEntry1, nsISHEntry* aEntry2)
>+  if ((!aEntry1 && aEntry2) || (aEntry1 && !aEntry2)) {

Perhaps |if (!aEntry1 != !aEntry2)| ?

Want to just return |bool| from this function?  Either way on that.

>+nsSHistory::RemoveDuplicate(PRInt32 aIndex)
>+    if (txNext) {
>+      txNext->SetPrev(txToKeep);
>+    }

This is handled by the txToKeep->SetNext impl already, right?

>+    txToRemove->SetNext(nsnull);
>+    txToRemove->SetPrev(nsnull);

I'd prefer we do this before |txToKeep->SetNext| in case someone makes SetNext null out the prev on the old next at some point.

> nsSHistory::RemoveEntries(nsTArray<PRUint64>& aIDs, PRInt32 aStartIndex)
>+      NS_NewRunnableMethod(static_cast<nsDocShell*>(mRootDocShell),
>+                           &nsDocShell::FireDummyOnLocationChange);

Why do we need to do that?

And if we're willing to cast here, why did we add the new nsIDocShell API?
> >+    } else if (aIndex < mPreviousTransIndex) {
> >+        mPreviousTransIndex = NS_MAX(-1, mPreviousTransIndex - 1);
> 
> Can aIndex be < -1?  If not, why is the NS_MAX with -1 needed?
Just to be safe. But I can change

> >+    } else if (aIndex < mLoadedTransIndex) {
> >+        mLoadedTransIndex = NS_MAX(0, mLoadedTransIndex - 1);
> >+    }
>  
> Can aIndex be < 0?
Same thing. This all is pretty much copied from the code above.
But indeed, I should change this and just assert invalid parameters.
> 
> Why is this still needed?  Or is it just a nicer-looking cleanup kinda thing?
Yeah, just a "clean-up"



> Can we put the new member at the end?  Or better yet on
> nsIDocShell_MOZILLA_2_0_BRANCH at this point....
ok.
> nsSHTransaction::SetNext(nsISHTransaction * aNext)
> > {
> >+  if (aNext) {
> >    NS_ENSURE_SUCCESS(aNext->SetPrev(this), NS_ERROR_FAILURE);
> 
> Fix the indent here.
Huh. I thought I fixed the indentation already, since the whole
method had strange indentation.

> Perhaps |if (!aEntry1 != !aEntry2)| ?
That looks rather strange to me.

 
> This is handled by the txToKeep->SetNext impl already, right?
Indeed.


> I'd prefer we do this before |txToKeep->SetNext| in case someone makes SetNext
> null out the prev on the old next at some point.
Ok.

 
> Why do we need to do that?
This all runs occasionally at unsafe time and we'd get a warning
about unsafe DOM Event since chrome modifies its DOM in some way.

> And if we're willing to cast here, why did we add the new nsIDocShell API?
Good question. I'll remove the method from .idl. This all is internal to the
docshell lib (even in non-libxul case).
> That looks rather strange to me.

OK, don't worry about it.

> This all runs occasionally at unsafe time and we'd get a warning
> about unsafe DOM Event since chrome modifies its DOM in some way

What I meant is, why do we need to call OnLocationChanged at all here?
Attached patch patchSplinter Review
Attachment #471471 - Attachment is obsolete: true
Attachment #471487 - Attachment is obsolete: true
Attachment #471812 - Flags: review?(bzbarsky)
Attachment #471487 - Flags: review?(bzbarsky)
Comment on attachment 471812 [details] [diff] [review]
patch

>+        mPreviousTransIndex = mPreviousTransIndex - 1;

--mPreviousTransIndex.  Same for mLoadedTransIndex there.

>+      mIndex = aIndex - 1;

Should be --mIndex, right?  Or at least mIndex on the RHS?

Please document that we need the FireDummyOnLocationChange to make the UI update?

r=me with that.  We should add some tests too, but I'm ok with doing that as a followup.
Attachment #471812 - Flags: review?(bzbarsky) → review+
Attached patch patchSplinter Review
http://hg.mozilla.org/mozilla-central/rev/b909c38edaf5

I'll add still some tests.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached patch a testSplinter Review
Duplicate of this bug: 651338
Alice: There is another Open dup at Bug 566048.

Rob, it has been fixed but only on 4.0+.  I'm unsure if this qualifies to be backported to the 1.9.2 tree.
> Alice: There is another Open dup at Bug 566048.

Not quite.  That bug is about changing the page (which we _do_ control) to work around this issue (e.g. so it doesn't happen in older Gecko-based browsers).

Kelley, the fix for this is not something we're willing to backport to a stable branch.  It was a pretty invasive change to the way session history works.
You need to log in before you can comment on or make changes to this bug.