Closed Bug 94514 Opened 19 years ago Closed 12 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta4

People

(Reporter: neil, Assigned: sdwilsh)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
argh.. take two at reassigning history bugs to new owner
Assignee: alecf → blakeross
Target Milestone: --- → mozilla1.2
Target Milestone: mozilla1.2 → Future
Attached patch possible patch (obsolete) — Splinter Review
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.
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.
*** Bug 152219 has been marked as a duplicate of this bug. ***
See also bug 97470, iframes/frames should not display in history.
Assignee: bross2 → nobody
QA Contact: claudius → history.global
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.
morphing bug to become a places bug
Component: History: Global → Places
OS: Windows 95 → All
Product: Core → Firefox
QA Contact: history.global → places
Hardware: PC → All
Target Milestone: Future → ---
one reason why this would be good for fx 3 is because of recent url bar autocomplete changes.
Depends on: 389491
Summary: POST result page should not appear in global history. → POST result page should not appear in global history or history autocomplete results
Actually, this bug is more serious than just the highlighted area: of the top six results, five are clearly POST results!
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
as discussed today, this causes a problem with the url bar autocomplete as posting could impact visit count.
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
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.
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Duplicate of this bug: 412958
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
Assignee: nobody → sdwilsh
Whiteboard: [needs patch]
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?
Status: NEW → ASSIGNED
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.
Attached patch v1.0 (obsolete) — Splinter Review
Not requesting review on this until I get tests written, but feel free to comment.

This works with case 1 at least for me.
Attachment #69914 - Attachment is obsolete: true
Attached patch v1.1 (obsolete) — Splinter Review
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...
Attachment #300808 - Attachment is obsolete: true
Attachment #300998 - Flags: superreview?(bzbarsky)
Attachment #300998 - Flags: review?(bzbarsky)
Whiteboard: [needs patch] → [has patch][needs r bz][needs sr bz]
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?
Attachment #300998 - Flags: superreview?(bzbarsky)
Attachment #300998 - Flags: superreview+
Attachment #300998 - Flags: review?(bzbarsky)
Attachment #300998 - Flags: review+
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?
Whiteboard: [has patch][needs r bz][needs sr bz] → [needs new patch][has r][has sr]
Attached patch v1.2 (obsolete) — Splinter Review
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.
Attachment #300998 - Attachment is obsolete: true
Attachment #301382 - Flags: review?(bzbarsky)
Whiteboard: [needs new patch][has r][has sr] → [has patch][needs r bz][has sr]
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?
(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 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.
Attachment #301382 - Flags: review?(bzbarsky) → review+
Attached patch v1.3Splinter Review
Fixes comment.  This is for checkin.
Attachment #301382 - Attachment is obsolete: true
Blocks: 415955
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs r bz][has sr]
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
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.