94514 - POST result page should not appear in global history or history autocomplete results

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[[e-mail]]

I was trying to look for a form in history (autocomplete) and accidentally
selected the form results page instead of the form itself. It doesn't make sense
to store the URL of the form results page in global history.

Comment 1

[Alec Flett]

argh.. take two at reassigning history bugs to new owner

Comment 2

[Blake Ross]

Attachment: possible patch

Comment 3

[Blake Ross]

I know very little about network, so I'm not sure if this patch is correct. 
Tested on a few POST forms.  I notice that IE does show such form results pages
in the UI.  Cc'ing bbaetz.

Comment 4

[Bradley Baetz (:bbaetz)]

This patch is slightly wonrg, becasue its possible for
http://foo.bar.com/xxx.cgi to be a page which posts to ittself. The first view
should be in global histroyy, the second one shouldn't. However, both should be
in local history.

POST result pages use a cache key which is != the document url, so I guess that
page should be hidden instead.

cc darin, who knows how that works.

Comment 5

[Jesse Ruderman]

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

Comment 6

[Jesse Ruderman]

See also bug 97470, iframes/frames should not display in history.

Comment 7

[Brett Wilson]

In addition to the concerns in comment 4, this patch is also a horrible hack. Rather than adding to history like normal, and then hiding the page, the POST state should be sent to history so the URL can be added with the correct codepath using the correct flags.

For Places, you probably want a new transition type in addition to TYPED, etc. that corresponds to POST requests. This is so even in the example where a page posts to itself, the first visit will appear in history properly, but the post won't.

Comment 8

[(not reading, please use [e-mail] instead)]

morphing bug to become a places bug

Comment 9

[(not reading, please use [e-mail] instead)]

one reason why this would be good for fx 3 is because of recent url bar autocomplete changes.

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of the bug

Comment 11

[Daniel Luz]

Actually, this bug is more serious than just the highlighted area: of the top six results, five are clearly POST results!

Comment 12

[(not reading, please use [e-mail] instead)]

as discussed today, this causes a problem with the url bar autocomplete as posting could impact visit count.

Comment 13

[(not reading, please use [e-mail] instead)]

with the upcoming change for bug #399477, this is going to be really noticable for bugzilla users, see https://bugzilla.mozilla.org/attachment.cgi?id=291339

Comment 14

[(not reading, please use [e-mail] instead)]

note, in the case of that scree shot, I've clearly also re-visited https://bugzilla.mozilla.org/post_bug.cgi upon restarting the browser after quit or crash, but we have a bug on not counting those visits as well.

Comment 16

[Shawn Wilsher :sdwilsh]

So, I'm working with two scenarios:
1) We post to a page, and it displays something given those results.  We DO NOT want to count this.
2) We post to a page, and it redirects us to some other page.  We DO want to count this.

Does anyone disagree with this?

Comment 17

[Jesse Ruderman]

If the redirect is to a "GET" and the POST was initiated by a clear user action (e.g. a click), counting it seems reasonable.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Not requesting review on this until I get tests written, but feel free to comment.

This works with case 1 at least for me.

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Here we go.  Now with a test.  I couldn't think of a way to test case two, but I'm open to suggestions.

Dietrich, Mano, and Seth:
Might this cause a problem in places with visit chains now?  We won't notice any pages with post results, so we'll be missing navigation paths (one could probably argue that in most cases it's a good thing).  For example:
1) Start off at https://bugzilla.mozilla.org/show_bug.cgi?id=94514
2) Make a change to the bug, you end up at https://bugzilla.mozilla.org/process_bug.cgi
3) Click a link to another bug on that page
We'll miss the page on step two, and step 1 and 3 will not be connected.  Is this bad?  I really don't know...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 300998 [details] [diff] [review]
v1.1

The code looks OK.  I have some issues with the test....

> 		bug404548-subframe.html \
>+		test_bug94514.html \

Please add it in the right place so that the tests are in order.

>+if (!window.location.href.match("posted=1")) {

It looks like you're doing SimpleTest.waitForExplicitFinish() in one Window, and calling finish() in another one.  I don't believe there's any guarantee that this would work.  Please just do the form submit in a different window from the one the test is running in (e.g. window.open a window and do the submit in there, or do it in a frame, or whatever).

>+  var form = document.getElementById("testForm");

$("testForm")

>+  // We need to check that this was not added to global history.

Shouldn't the test start make sure to remove it from global history or something?  Otherwise if this ever regresses, the test will keep failing even if the regression is fixed.

r+sr=bzbarsky on the code, but please fix up the test, ok?

Comment 21

[Shawn Wilsher :sdwilsh]

Alright, I can fix those.  Admittedly, this was my first mochitest.

What do you mean by "in order"?  In order of bug numbers or what?

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Carrying forward sr

(In reply to comment #20)
> Shouldn't the test start make sure to remove it from global history or
> something?  Otherwise if this ever regresses, the test will keep failing even
> if the regression is fixed.
There's no API to remove something from global history, but I check and make sure it's not added first.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Uh... how can you assert that the initial page is not in global history?  That might be true for a fully automated mochitest run, but for someone running them by hand (multiple times in a row on the same profile, in that case), that would simply fail, no?

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> Uh... how can you assert that the initial page is not in global history?  That
> might be true for a fully automated mochitest run, but for someone running them
> by hand (multiple times in a row on the same profile, in that case), that would
> simply fail, no?
Yeah, you can only run it once.  It sucks, but how do I remove it from global history exactly?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 301382 [details] [diff] [review]
v1.2

OK, but please change the messages to make it clear that the error could just be from running twice without a restart.

And perhaps file a bug on improving the history API.

Comment 26

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Fixes comment.  This is for checkin.

Comment 27

[Shawn Wilsher :sdwilsh]

So, I talked with dietrich about comment 19.  While this patch breaks session tracking, no user facing feature uses that yet.  In addition, we don't expect there to be one in time for 1.9/Fx3.  The functionality that this will provide, however, is a big win for users (anyone who uses bugzilla should be able to attest to that).  So, we'll do a follow-up (bug 415955) to get nsIGlobalHistory extended again to better track this for the next major release.

Checking in docshell/base/nsDocShell.cpp;
new revision: 1.884; previous revision: 1.883
Checking in docshell/test/Makefile.in;
new revision: 1.11; previous revision: 1.10
Checking in docshell/test/bug94514-postpage.html;
initial revision: 1.1
Checking in docshell/test/test_bug94514.html;
initial revision: 1.1

Comment 28

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032106 Minefield/3.0b5pre
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008032106 Minefield/3.0b5pre

Comment 29

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv