Last Comment Bug 680727 - "Try again" in an error page does not write it in global history
: "Try again" in an error page does not write it in global history
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: 6 Branch
: All All
: -- normal (vote)
: mozilla14
Assigned To: O. Atsushi (Torisugari)
:
Mentors:
: 685601 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-20 21:07 PDT by O. Atsushi (Torisugari)
Modified: 2012-03-29 08:52 PDT (History)
7 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple propsal (7.03 KB, patch)
2011-10-17 06:33 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple propsal + comment fix (7.17 KB, patch)
2011-10-17 06:48 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal + comment fix (7.59 KB, patch)
2011-10-18 09:59 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal + comment fix (7.28 KB, patch)
2011-10-18 10:18 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal + comment fix + b-c test (13.97 KB, patch)
2011-11-02 03:28 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal + comment fix + b-c test (15.06 KB, patch)
2011-11-04 04:26 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal v7 (17.82 KB, patch)
2011-11-29 11:35 PST, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal v8 (18.43 KB, patch)
2011-11-30 08:03 PST, O. Atsushi (Torisugari)
mak77: feedback+
Details | Diff | Review
Simple proposal v9 part 1 (toolkit/places) (10.50 KB, patch)
2011-12-10 17:20 PST, O. Atsushi (Torisugari)
mak77: review+
Details | Diff | Review
Simple proposal v9 part 2 (docshell) (10.39 KB, patch)
2011-12-10 17:54 PST, O. Atsushi (Torisugari)
justin.lebar+bug: feedback+
Details | Diff | Review
Simple proposal v9.1 part 1 (toolkit/components/places) (10.46 KB, patch)
2011-12-13 02:49 PST, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal v9.2 part 1 (toolkit/components/places) (10.83 KB, patch)
2011-12-13 06:54 PST, O. Atsushi (Torisugari)
mak77: review+
Details | Diff | Review
Simple proposal v10 part 2 (docshell) (6.21 KB, patch)
2012-03-21 10:28 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
Simple proposal v10.1 part 2 (docshell) (6.21 KB, patch)
2012-03-21 10:35 PDT, O. Atsushi (Torisugari)
bugs: review+
justin.lebar+bug: feedback+
Details | Diff | Review
Simple proposal v10.2 part 2 (docshell) (7.28 KB, patch)
2012-03-22 08:00 PDT, O. Atsushi (Torisugari)
bugs: review+
Details | Diff | Review

Description O. Atsushi (Torisugari) 2011-08-20 21:07:43 PDT
Steps to reproduce:

  Purple link: A HTML anchor whose target is recorded in global history.
  Blue link:   A HTML anchor whose target is not recorded in global history.

1. Go to a web page, which contains some blue links.
2. Disable network connection of the machine (pull out LAN cable, etc.)
3. Click a blue link, and have the browser show the error page.
4. Enable network connection.
5. Click "Try again", and have the browser show the target page.

Expected result:
The target page is listed in the history menu, etc.

Actual result:
The target page is not listed in the history menu. If you go back to the original page, the link is still blue.

My guess:
"Try again" is equivalent to "location.reload()" so the request should have LOAD_CMD_RELOAD flag. That's why nsDocShell::OnNewURI(...) fails to call nsDocShell::AddURIVisit(...). As a result, the browser does not remember whether or not the user have been to the page, even when the user actually visited the page.

By the way, this happens very often when the network connection is unstable. It's a kind of dataloss.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-20 21:17:02 PDT
Seems like we should ignore the reload flag for AddURIVisit if the current page is an error page?
Comment 2 O. Atsushi (Torisugari) 2011-08-21 03:43:09 PDT
Ideally, in my opinion, "Try Again" should simulate the first (failed) request. If the first request has the reload flag, the second request should have the reload flag.

> Seems like we should ignore the reload flag for AddURIVisit if the current
> page is an error page?

So this behavior is inconsistent, and calling AddURIVisit too many times. However, I'm afraid the difference is too trivial for any users to notice. In the end, it should depend on how easy to implement/maintain it.

By the way, I'm not sure why normal reload should not hit global history. What's wrong with that clicking the "reload" button updates last-visit date?
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-22 08:10:27 PDT
No idea.... ask the Places folks?
Comment 4 O. Atsushi (Torisugari) 2011-10-17 02:20:34 PDT
*** Bug 685601 has been marked as a duplicate of this bug. ***
Comment 5 :aceman 2011-10-17 03:21:22 PDT
Places folk commented in bug 685601 comment 3, that it doesn't seem to be problem in Places and changed component into Document navigation (as it is here).
What to do next?
Comment 6 O. Atsushi (Torisugari) 2011-10-17 06:08:56 PDT
(In reply to aceman from comment #5)
> Places folk commented in bug 685601 comment 3, that it doesn't seem to be
> problem in Places and changed component into Document navigation (as it is
> here).

Yes, but what I suggested here is:

  Not only error pages' reload, all of the reload should notified to
  global history, for the sake of simple coding.

Places people don't know this plan.

If they denied it, we will just go in a little complicated way as is proposed at comment 1.

>What to do next?
Lobbying...?
Comment 7 O. Atsushi (Torisugari) 2011-10-17 06:33:11 PDT
Created attachment 567433 [details] [diff] [review]
Simple propsal

docshell gives global-history notice of every reload.
Comment 8 :aceman 2011-10-17 06:42:11 PDT
Does this work for you?
Then request review of the patch from :bz .
Comment 9 O. Atsushi (Torisugari) 2011-10-17 06:48:01 PDT
Created attachment 567435 [details] [diff] [review]
Simple propsal + comment fix
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-17 07:21:37 PDT
Seriously, the global history notification code in docshell is written around the requirements Places imposes.  So if you want to change it, you need to check with the Places people to make sure their code can deal.
Comment 11 O. Atsushi (Torisugari) 2011-10-18 09:59:08 PDT
Created attachment 567781 [details] [diff] [review]
Simple proposal + comment fix

"Bug 680727 -- DocShell gives global-history notice of every reload."

About the XXX comment, I read bug 89628 several times again, but I'm not yet sure why. However, it has little to do with this bug itself, so I'll leave it to anyone else who understand it.

The first disadvantage I've noticed so far is that F5 attackers may crush their own machine (or browser's instance) due to too many SQL sentences within a tiny duration. Is this realistic?

The second is that if a website has a script something like location.reload() + setInterval()/setTimeout(), users may lose their actual last-visit dates. As well as <meta> element's http refresh.
Comment 12 O. Atsushi (Torisugari) 2011-10-18 10:18:44 PDT
Created attachment 567784 [details] [diff] [review]
Simple proposal + comment fix

"Bug 680727 -- DocShell gives global-history notice of every reload."

sigh.
Comment 13 Marco Bonardo [::mak] 2011-10-19 05:58:04 PDT
(In reply to aceman from comment #5)
> Places folk commented in bug 685601 comment 3, that it doesn't seem to be
> problem in Places and changed component into Document navigation (as it is
> here).

What I said is that Places just registers any request it gets, if it doesn't get a request, it won't register it in history. Docshell has the duty to send that request where appropriate, thus I think the bug should live here.

That said, it's a thing to look into both sides, docshell has to send the request, and if too many requests generate heavy load or issues on the db, we may want to do some caching Places side. The concept of a Places team is dead quite some time ago, changes must be coordinated and discussed in platform and toolkit to find best solutions. This is not a ping pong game :)

I don't think F5-attacks are something we really should be worried about, the user is unlikely fast enough to beat the computer.
But if that happens for script reloads we are in trouble, would be easy to create DOS pages that way.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-19 07:28:29 PDT
Marco, docshell has all sorts of code to avoid sending requests that Places apparently can't handle for various reasons.  I won't pretend to understand the logic behind it, but I'd like someone who _does_ understand it figure out what the right thing to do on reloads is.

It's not a matter of ping-pong; it's a matter of code ownership.  And the global history notification code in docshell wasn't written by and isn't really owned by any of the people who normally work on docshell; it was more or less custom designed around the needs of Places.  So all I want is for someone familiar with that design to deal with reviewing this patch, since they would do a way better job than I would.
Comment 15 O. Atsushi (Torisugari) 2011-10-19 08:30:42 PDT
(In reply to Marco Bonardo [:mak] from comment #13)
> But if that happens for script reloads we are in trouble, would be easy to
> create DOS pages that way.

If I remember correctly, history.pushState(...) updates both session history and global history. So now a script on www seems to me able to DOS-attack our SQL database, without any reload...
Comment 16 Justin Lebar (not reading bugmail) 2011-10-19 09:02:26 PDT
(In reply to O. Atsushi (Torisugari) from comment #15)
> If I remember correctly, history.pushState(...) updates both session history
> and global history.

Correctamundo.  And we don't (currently) do any rate-limiting.
Comment 17 Marco Bonardo [::mak] 2011-10-25 08:38:15 PDT
(In reply to Boris Zbarsky (:bz) from comment #14)
> Marco, docshell has all sorts of code to avoid sending requests that Places
> apparently can't handle for various reasons.

Places was built as a replacement for mork history, retaining most of the existing APIs. Clearly we can make nicer APIs where needed, but I'm not that far from your knowledge. Nobody who designed this interaction is still here, and I have no idea why certain things have been done in certain ways. I think something improved with async history API (IHistory), that, as of now, is still experimental, so we can change it. Btw, this sounds like OT, let's stay on the bug.

Here is 2 separate problems:
- figure out how to handle reloads
- avoid possibility for content to flood history
According to previous comments looks like the latter is already an issue, that by itself does not mean it should be ignored. We should likely write flooding tests and see what happens, and see what we can do to limit the problem.
Comment 18 Marco Bonardo [::mak] 2011-10-25 09:01:41 PDT
Comment on attachment 567784 [details] [diff] [review]
Simple proposal + comment fix

Review of attachment 567784 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is pretty fine, I still want to think a bit about what happens after the AddURIVisit call though.
This really wants a test, a b-c test is likely fine, and I honestly have no idea if we already have an opposite test checking reloads are NOT added. You may do a tryrun on Linux only to check it out.

::: docshell/base/nsDocShell.cpp
@@ +9278,5 @@
>          // Get the handle to SH from the root docshell          
>          GetRootSessionHistory(getter_AddRefs(rootSH));
> +        if (!rootSH) {
> +            updateSessionHistory = false;
> +            updateGlobalHistory = false; /* XXX Why global history too? */

good question, it looks wrong. We should likely just fix it.
Let me see, this was done in bug 89628. This is the patch https://bug89628.bugzilla.mozilla.org/attachment.cgi?id=41894 (this is archeology)

@@ +9342,5 @@
>           aLoadType == LOAD_RELOAD_BYPASS_PROXY ||
>           aLoadType == LOAD_RELOAD_BYPASS_PROXY_AND_CACHE)) {
> +        NS_ASSERTION(!updateSessionHistory,
> +                     "We shouldn't be updating session history for forced"
> +                     " reloads!");

Places is slowly converting touched NS_ASSERTIONs to aborting MOZ_ASSERT, since assertions must be real and noticed! I wonder if docshell wants to do that, Boris?

@@ +9385,5 @@
> +       Update session history.
> +
> +       This is a fresh page getting loaded for the first time. Create a Entry
> +       for it and add it to SH, if this is the rootDocShell.
> +     */

nit: I'd rather prefer line comments style, it makes easier to comment out parts of code when debugging... docshell has mixed styles for comments, but it should likely be either // on each line or like this but with * on each line, like other comments in the same file.

@@ +9387,5 @@
> +       This is a fresh page getting loaded for the first time. Create a Entry
> +       for it and add it to SH, if this is the rootDocShell.
> +     */
> +    if (updateSessionHistory && !mLSHE && (mItemType == typeContent) &&
> +        mURIResultedInDocument) { 

may we coalesce more of these in the updateSessionHistory definition above so here we just have if (updateSessionHistory) ?

@@ +9389,5 @@
> +     */
> +    if (updateSessionHistory && !mLSHE && (mItemType == typeContent) &&
> +        mURIResultedInDocument) { 
> +        (void) AddToSessionHistory(aURI, aChannel, aOwner, aCloneSHChildren,
> +                                   getter_AddRefs(mLSHE));

nit: remove space between (void) and the function

@@ +9398,5 @@
> +
> +       If this is a POST request, we do not want to include this in global
> +       history.
> +     */
> +    if (updateGlobalHistory && !ChannelIsPost(aChannel)) {

ditto, if we can coalesce conditions above, may be easier to check when something happens without bouncing up and down
Comment 19 O. Atsushi (Torisugari) 2011-11-02 03:28:49 PDT
Created attachment 571293 [details] [diff] [review]
Simple proposal + comment fix + b-c test

Addressing every issues in comment #18

Except for
> I honestly have no idea if we already have an opposite test checking
> reloads are NOT added. You may do a tryrun on Linux only to check it out.

For my tree did not pass full-tests even when no patch was applied.
Comment 20 O. Atsushi (Torisugari) 2011-11-03 02:47:58 PDT
After washing tons of test failure logs, I finally found bug 399606 (and bug 330138). That's what you mentioned, right? Certainly this patch makes unit-test orange.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/./toolkit/components/places/tests/browser/browser_bug399606.js | onVisit has been received right number of times for http://example.com/tests/toolkit/components/places/tests/browser/399606-location.reload.html

According to attachment #300728 [details] [diff] [review], nsNavHistory compares request URI and its referrer to avoid duplication. On the other hand, the referrer of a reload request is that of the original request. Then, from docshell side, maybe it's easy to fix bug 399606 because we have existing |equalUri| flag...
Comment 21 O. Atsushi (Torisugari) 2011-11-04 04:26:17 PDT
Created attachment 571931 [details] [diff] [review]
Simple proposal + comment fix + b-c test

To make both tests pass.
Comment 22 Marco Bonardo [::mak] 2011-11-26 06:09:06 PST
Comment on attachment 571931 [details] [diff] [review]
Simple proposal + comment fix + b-c test

Review of attachment 571931 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/History.cpp
@@ +1681,5 @@
>      if (same) {
> +      // Do not save refresh-page visits if we have already visited |aURI|.
> +      bool isVisited = true;
> +      rv = navHistory->IsVisited(aURI, &isVisited);
> +      NS_ENSURE_SUCCESS(rv, rv);

Unfortunately we can't do this, IsVisited() is synchronous and this is an asynchronous API, we can't mix the two unless we want serious troubles. There is no way you can know synchronously here if there is already a visit, so this will require some deeper change in History.cpp.
Or we may cache the previous successful visit addition and compare with it, may satisfy your case.
Comment 23 O. Atsushi (Torisugari) 2011-11-26 07:33:53 PST
Hmm.

(In reply to Marco Bonardo [:mak] from comment #22)
> There is no way you can know synchronously here if there is already a visit,
> so this will require some deeper change in History.cpp.

Is it possible to get something informative enough from |navHistory->GetRecentFlags(aURI)|, instead of isVisited()? Like
http://hg.mozilla.org/mozilla-central/annotate/36b62de19e9e/toolkit/components/places/History.cpp#l1702

I expect these flags (and corresponding URIs) are cached in a nice way. No?

> Or we may cache the previous successful visit addition and compare with it,
> may satisfy your case.

At docshell side, we know whether a request is RELOAD (or not RELOAD). If I set null to |aLastVisitURI|, the |same| will be false. Therefore we can bypass this block easily.

>::: toolkit/components/places/History.cpp
>@@ +1681,5 @@
>>      if (same) {
>> +      // Do not save refresh-page visits if we have already visited |aURI|.
>> +      bool isVisited = true;
>> +      rv = navHistory->IsVisited(aURI, &isVisited);
>> +      NS_ENSURE_SUCCESS(rv, rv);

The demerit of this method is that we have to care about too much reloads again. And probably we should create a new trasition flag, say, nsINavHistoryService::TRANSITION_RELOAD or something, to prevent frecency scores of the reqest from getting too high. That's not too difficult, IMO.
Comment 24 Marco Bonardo [::mak] 2011-11-26 07:59:34 PST
(In reply to O. Atsushi (Torisugari) from comment #23)
> Is it possible to get something informative enough from
> |navHistory->GetRecentFlags(aURI)|, instead of isVisited()? Like
> http://hg.mozilla.org/mozilla-central/annotate/36b62de19e9e/toolkit/
> components/places/History.cpp#l1702
> 
> I expect these flags (and corresponding URIs) are cached in a nice way. No?

That would be fast, but not all visits set a flag, only specific ones (like typed or those from bookmarks)

> > Or we may cache the previous successful visit addition and compare with it,
> > may satisfy your case.
> 
> At docshell side, we know whether a request is RELOAD (or not RELOAD). If I
> set null to |aLastVisitURI|, the |same| will be false. Therefore we can
> bypass this block easily.

But this would be another docshell hack, I think we probably have enough of those. Btw, just to understand, you'd null aLastVisitURI when RELOAD is false? since otherwise...

> >::: toolkit/components/places/History.cpp
> >@@ +1681,5 @@
> >>      if (same) {
> >> +      // Do not save refresh-page visits if we have already visited |aURI|.
> >> +      bool isVisited = true;
> >> +      rv = navHistory->IsVisited(aURI, &isVisited);
> >> +      NS_ENSURE_SUCCESS(rv, rv);
> 
> The demerit of this method is that we have to care about too much reloads
> again. And probably we should create a new trasition flag, say,
> nsINavHistoryService::TRANSITION_RELOAD or something, to prevent frecency
> scores of the reqest from getting too high. That's not too difficult, IMO.

...I was exactly thinking that way you'd bypass the internal protection for reloads. I'm not just worried by frecency, and surely adding a transition is not too problematic (apart the need to write and modify a bunch of tests), but we'd also lose protection from database and storage thread flooding.

I think I like your solution to skip the protection when the visit is not in history, we just need to make that fast. In History.cpp there is the code that notifies of a visit addition, we may cache the last addition there.
Comment 25 O. Atsushi (Torisugari) 2011-11-26 12:08:12 PST
(In reply to Marco Bonardo [:mak] from comment #24)
> But this would be another docshell hack, I think we probably have enough of
> those. Btw, just to understand, you'd null aLastVisitURI when RELOAD is
> false? since otherwise...

In my last patch...
>+    // Update global history. 
>+    //
>+    // We do update global history on a reload request, while we don't create
>+    // a new session history entry. See bug 680727.
>+    //
>+    // If this is a POST request, we do not want to include this in global
>+    // history.
>+    if (aAddToGlobalHistory &&
>+        aLoadType != LOAD_BYPASS_HISTORY &&
>+        aLoadType != LOAD_ERROR_PAGE &&
>+        !(aLoadType & LOAD_CMD_HISTORY) &&
>+        !ChannelIsPost(aChannel)) {
>+#ifdef DEBUG
>+        updateGlobalHistory = true;
>+#endif
>+        nsCOMPtr<nsIURI> previousURI;
>+        PRUint32 previousFlags = 0;
>+
>+        if (aLoadType & LOAD_CMD_RELOAD) {
>+            // On a reload request, we don't set redirecting flags.
>+            previousURI = aURI;
>+        } else {
>+            ExtractLastVisit(aChannel, getter_AddRefs(previousURI),
>+                             &previousFlags);
>+        }
>+
>+        // Note that we don't use referrer when our global history is based on
>+        // IHistory.
>+        nsCOMPtr<nsIURI> referrer;
>+        // Treat referrer as null if there is an error getting it.
>+        (void)NS_GetReferrerFromChannel(aChannel, getter_AddRefs(referrer));
>+
>+        AddURIVisit(aURI, referrer, previousURI, previousFlags);
>+    }

On a reload request, I expect |nsDochsell::ExtractLastVisit(nsIChannel*)| returns the currentURI, but it does not. In other words, reload is not nsIChannel's bussiness. Reload event is inside of docshell. That's why I need this hacky part anyway.

>+        if (aLoadType & LOAD_CMD_RELOAD) {
>+            // On a reload request, we don't set redirecting flags.
>+            previousURI = aURI;
>+        } else {

If I set |previousURI=nsnull| or |previousURI=referrer|, it will bypass the History.cpp's protection. On the other hand, HTTP header's refresh and <meta> tag's refresh are channel's business. And docshell sets the last URI through |nsDocShell::SaveLastVisit(nsIChannel*)|. So |nsDochsell::ExtractLastVisit(nsIChannel*)| returns the last URI which is equals to currentURI. In other cases, "the last URI" is the referrer.


> I think I like your solution to skip the protection when the visit is not in
> history, we just need to make that fast. In History.cpp there is the code
> that notifies of a visit addition, we may cache the last addition there.

It's not clear to me what the "cache" is like. I guess you are talking about here:
http://hg.mozilla.org/mozilla-central/annotate/36b62de19e9e/toolkit/components/places/History.cpp#l1749

Then, it is something like

 mLastVisited = aURI;

or a hash table? What happens when we have 2 tabs opened (and 2 docshells exist at a time)?
Comment 26 Marco Bonardo [::mak] 2011-11-29 02:36:01 PST
By notification I mean adding some caching in History::NotifyVisited(nsIURI* aURI)

It's possible we may want something like a ring buffer, storing the last 10 visited uris.
Comment 27 Marco Bonardo [::mak] 2011-11-29 02:38:10 PST
Comment on attachment 571931 [details] [diff] [review]
Simple proposal + comment fix + b-c test

Clearing the flag while we figure out the above
Comment 28 O. Atsushi (Torisugari) 2011-11-29 11:35:53 PST
Created attachment 577688 [details] [diff] [review]
Simple proposal v7

Adding Something like a Ring-buffer.

History::NotifyVisited(nsIURI* aURI) seems by far more frequently called than History::VisitURI(...). I'm not sure, but probably they are per the number of visited links in one document.
Comment 29 Marco Bonardo [::mak] 2011-11-29 15:18:38 PST
you may try using NotifyVisitObservers::Run then, since NotifyVisited is likely serving also the links, as you reported.
Comment 30 O. Atsushi (Torisugari) 2011-11-30 08:03:24 PST
Created attachment 577965 [details] [diff] [review]
Simple proposal v8

(In reply to Marco Bonardo [:mak] from comment #29)
> you may try using NotifyVisitObservers::Run then, since NotifyVisited is
> likely serving also the links, as you reported.

Thanks. That is working as expected.

Then the last issue in my mind is whether or not we should use this ring-buffer for another kind of maybe-too-many requests; e.g. history.pushState(). Now. there's little demerit to keep this cache full. But if we use it for another purpose, we have to clear that cache together with user's clearing the global history. Otherwise, the global history will stop working for a while after the sanitizing dialog etc.
Comment 31 Marco Bonardo [::mak] 2011-12-02 05:28:13 PST
(In reply to O. Atsushi (Torisugari) from comment #30)
> Then the last issue in my mind is whether or not we should use this
> ring-buffer for another kind of maybe-too-many requests; e.g.
> history.pushState(). Now. there's little demerit to keep this cache full.
> But if we use it for another purpose, we have to clear that cache together
> with user's clearing the global history. Otherwise, the global history will
> stop working for a while after the sanitizing dialog etc.

This can be handled apart in another bug, right? If so, please just file it apart.
Comment 32 Marco Bonardo [::mak] 2011-12-02 07:06:41 PST
Comment on attachment 577965 [details] [diff] [review]
Simple proposal v8

Review of attachment 577965 [details] [diff] [review]:
-----------------------------------------------------------------

Apart a couple minor things it looks good to me... I'd appreciate if bz could do a pass on next iteration, since I'm not a docshell peer.
I'll take just a quick look at it, I think I'm covering most of my concerns here.

::: docshell/base/nsDocShell.cpp
@@ +9353,5 @@
>          ClearFrameHistory(mLSHE);
>          ClearFrameHistory(mOSHE);
>      }
>  
> +    nsCOMPtr<nsISHistory> rootSH = mSessionHistory;

may we find a more self-documenting name than rootSH? Could we just call it sessionHistory?

@@ +9360,5 @@
> +        GetRootSessionHistory(getter_AddRefs(rootSH));
> +    }
> +
> +#ifdef DEBUG
> +    bool shAvailable = rootSH != nsnull;

why not bool shAvailable = !rootSH;? Btw, if we find a better name for rootSH this is likely useless.

::: docshell/test/browser/browser_bug680727.js
@@ +5,5 @@
> +   global history. See bug 680727. */
> +/* TEST_PATH=docshell/test/browser/browser_bug680727.js make -C $(OBJDIR) mochitest-browser-chrome */
> +
> +
> +const kUniqueURI = "http://example.com/#THIS_IS_A_UNIQUE_URI";

please use http://mochi.test:8888/ instead of example.com

@@ +11,5 @@
> +function isVisited(aURI) {
> +  return Components.classes["@mozilla.org/browser/nav-history-service;1"]
> +                   .getService(Components.interfaces.nsINavHistoryService)
> +                   .isVisited(Services.io.newURI(aURI, null, null));
> +}

This runs in browser, so you can use PlacesUtils.history.isVisited... but see below first.

@@ +21,5 @@
> +
> +  // Clear network cache.
> +  Components.classes["@mozilla.org/network/cache-service;1"]
> +            .getService(Components.interfaces.nsICacheService)
> +            .evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);

Since you are running in browser you can use Cc, Ci... those shortcuts are unlikely to be ever removed.

@@ +53,5 @@
> +  // But location bar should show the original request.
> +  is(content.location.href, kUniqueURI, "Request is: " + kUniqueURI);
> +
> +  // Global history does not record URI of a failed request.
> +  ok(!isVisited(kUniqueURI), "This request is not listed in global history.");

Note that visits addition is asynchronous, so it's not enough to call isVisited, you should copy this method in your test and use it

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/head.js#64

and at that point I'd prefer if you'd use mozIAsyncHistory.isURIVisited than isVisited (the latter should be deprecated since it's synchronous)

So you should do whatever you need to cause a visit addition, then something like
waitForAsyncUpdates(function() {
  asyncHistory.isURIVisited(uri, callback);
});

::: toolkit/components/places/History.cpp
@@ +1314,5 @@
>  
>  History* History::gService = NULL;
>  
>  History::History()
> +  : mShuttingDown(false), mRecentlyVisitedURIsNextIndex(0)

on newline like
 : mShuttingdown...
 , mRecentlyVisited...

@@ +1650,5 @@
>  }
>  
> +void
> +History::AppendToRecentlyVisitedURIs(nsIURI* aURI) {
> +  // The max length of mRecentlyVisitedURIs is 8.

The length should be moved to a #define so we can easily change it in future (clearly the code should adapt to its value).

I think naming the define properly will make it self documenting and this comment can go away. rather may be interesting to state when the buffer is full we reuse existing entries.

@@ +1654,5 @@
> +  // The max length of mRecentlyVisitedURIs is 8.
> +  if (mRecentlyVisitedURIs.Length() < 8) {
> +    mRecentlyVisitedURIs.AppendElement(aURI);
> +  } else {
> +    mRecentlyVisitedURIsNextIndex &= 0x07;

maybe, to simplify using a single define, you may just use a %= 8 here

@@ +1671,5 @@
> +    aURI->Equals(mRecentlyVisitedURIs.ElementAt(i), &equals);
> +    if (equals) {
> +      break;
> +    }
> +  }

This is more compact:

bool equals = false;
for (nsTArray<nsCOMPtr<nsIURI> >::size_type i = 0;
     i < mRecentlyVisitedURIs.Length() && !equals; ++i) {
  aURI->Equals(mRecentlyVisitedURIs.ElementAt(i), &equals);
}
return equals;

::: toolkit/components/places/History.h
@@ +209,5 @@
> +  /**
> +   * mRecentlyVisitedURIs remembers the last 8 URIs to avoid saving these
> +   * locations repeatedly in a short period.
> +   */
> +  nsTArray<nsCOMPtr<nsIURI>> mRecentlyVisitedURIs;

This probably doesn't compile on Windows due to the >> you have to put a space in the middle of those as nsTArray<nsCOMPtr<nsIURI> >

@@ +210,5 @@
> +   * mRecentlyVisitedURIs remembers the last 8 URIs to avoid saving these
> +   * locations repeatedly in a short period.
> +   */
> +  nsTArray<nsCOMPtr<nsIURI>> mRecentlyVisitedURIs;
> +  PRInt32 mRecentlyVisitedURIsNextIndex;

I think you could use nsAutoTArray of length 8 here, since we enforce the length, should be more memory efficient.

The comment above should refer to the define name rather than the hardcoded value of 8.
Comment 33 Marco Bonardo [::mak] 2011-12-02 07:07:59 PST
> why not bool shAvailable = !rootSH;?

Clearly I meant bool shAvailable = !!rootSH; but I was too lazy and forgot a "!"
Comment 34 Marco Bonardo [::mak] 2011-12-02 07:10:42 PST
Ah, at the end of the test you should remove your entry from history, look around for the waitForClearHistory helper and copy it into your test. note you can't use it in registerCleanupFunction since it doesn't allow asynchronous methods, just use it as waitForClearHistory(finish); and we'll have to survive not cleaning in case of failure, for now.
Comment 35 O. Atsushi (Torisugari) 2011-12-10 17:20:16 PST
Created attachment 580703 [details] [diff] [review]
Simple proposal v9 part 1 (toolkit/places)

Then I think I should have split into places | docshell more earlier.

Addressing comment #32 and #34, and moving test from docshell to places.

By the way, I wonder why ns"Auto"TArray can be a member variable. Or is it doing type-inference?
Comment 36 O. Atsushi (Torisugari) 2011-12-10 17:54:22 PST
Created attachment 580707 [details] [diff] [review]
Simple proposal v9 part 2 (docshell)

Let me explain what I'm doing here:

The goal of this patch is to
1. Make global history and session history separated.
2. At global history, remove checking LOAD_CMD_HISTORY.
though I didn't mean to such a long patch...

Now this is comparing
> aLoadType != LOAD_BYPASS_HISTORY
> aLoadType != LOAD_ERROR_PAGE
twice.

Instead, checking
> aLoadType & (LOAD_CMD_HISTORY | LOAD_CMD_RELOAD)
only once.

This is treating |aLoadType| and |mLoadType| equally. And indeed these are basically identical unless we are going to compare it with LOAD_NORMAL, LOAD_LINK, LOAD_STOP_CONTENT and LOAD_NORMAL_REPLACE. For
>    if (equalUri &&
>        mOSHE &&
>        (mLoadType == LOAD_NORMAL ||
>         mLoadType == LOAD_LINK ||
>         mLoadType == LOAD_STOP_CONTENT) &&
>        !inputStream)
>    {
>        mLoadType = LOAD_NORMAL_REPLACE;
>    }

About 
>+        if (aLoadType & LOAD_CMD_RELOAD) {
>+            // On a reload request, we don't set redirecting flags.
>+            previousURI = aURI;
>+        } else {
please take a look at comment #25.

Other parts are conservative, relatively.

(In reply to Marco Bonardo [:mak] from comment #32)
> ::: docshell/base/nsDocShell.cpp
> @@ +9353,5 @@
> >          ClearFrameHistory(mLSHE);
> >          ClearFrameHistory(mOSHE);
> >      }
> >  
> > +    nsCOMPtr<nsISHistory> rootSH = mSessionHistory;
> 
> may we find a more self-documenting name than rootSH? Could we just call it
> sessionHistory?

If I understand right, |mSessionHistory| has a meaningful value only when this docShell is the root docshell. That is, in any case, |rootSH| is the root docshell's session history. And nsDocShell.cpp adopts "rootSH" here and there.
Comment 37 O. Atsushi (Torisugari) 2011-12-10 18:06:58 PST
(In reply to O. Atsushi (Torisugari) from comment #36)
> The goal of this patch is to
> 1. Make global history and session history separated.
> 2. At global history, remove checking LOAD_CMD_HISTORY.
> though I didn't mean to such a long patch...

s/LOAD_CMD_HISTORY/LOAD_CMD_RELOAD/

Nothing around LOAD_CMD_HISTORY will change. This bug is about reload.
Comment 38 Justin Lebar (not reading bugmail) 2011-12-10 18:07:57 PST
> By the way, I wonder why ns"Auto"TArray can be a member variable. Or is it doing 
> type-inference?

I'm not sure what you mean by type-inference.  There are three classes at work here: nsTArray, nsAutoTArray, and nsTArrayHeader.  Roughly speaking, they work like this:

  struct nsTArrayHeader<T> {
    PRUint32 mLength;
    PRUint32 mCapacity;
    T mData[0];
  };

  struct nsTArray<T> {
    nsTArrayHeader<T> *mHdr;
  };

  struct nsAutoTArray<T, N> {
    nsTArrayHeader<T> *mHdrPtr;
    nsTArrayHeader<T>  mInlineHeader;
    T                  mInlineData[N];
  };

When we malloc nsTArrayHeader, we create extra space at the end for |mCapacity| elements in the mData array.

nsAutoTArray stores data in mInlineData while there's enough space.  But if it gets too big, it switches over to using mHdrPtr, at which point it acts just like a normal nsTArray.

So you can use nsAutoTArray as a class member just like nsTArray.  (The only exception is that you can't memmove an nsAutoTArray.)
Comment 39 O. Atsushi (Torisugari) 2011-12-10 18:27:35 PST
(In reply to Justin Lebar [:jlebar] from comment #38)
Thank you for clearing.

> I'm not sure what you mean by type-inference.
I think ns"Auto"String is named because it must be an auto variable. e.g.

void foo() {
  auto int a = 0;
  nsCAutoString b("ABC");
}

These days, "auto" is used in a different way (type-inference):

void bar {
  int a = 1;
  int b = 2;
  auto c = a + b;
}

So I did like to know what "auto" means.
Comment 40 Justin Lebar (not reading bugmail) 2011-12-10 18:37:03 PST
Actually, nsAutoString is the same sort of thing as nsAutoTArray, and there's no technical restriction (that I'm aware of) against using it as a class member.  But we usually avoid using nsAutoSting / nsAutoArray as a class class since you always pay for the extra storage, even when the string/array is empty.
Comment 41 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-11 14:30:27 PST
The use of "auto" in type names like nsCAutoString and nsAutoTArray is horribly confusing.  It suggests that they are stack allocated but they are not.  They should be called nsCInlineString and nsInlineTArray, because the data is stored inline within the header.

> These days, "auto" is used in a different way (type-inference):

That's not right either.  "auto" basically just means "local" and typically is allocated on the stack.  See http://en.wikipedia.org/wiki/Automatic_variable.  The fact that C++11 uses it in combination with type inference is a separate issue.
Comment 42 Marco Bonardo [::mak] 2011-12-12 09:06:50 PST
Comment on attachment 580703 [details] [diff] [review]
Simple proposal v9 part 1 (toolkit/places)

Review of attachment 580703 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/History.h
@@ +213,5 @@
> +   * mRecentlyVisitedURIs remembers URIs which are recently added to the DB,
> +   * to avoid saving these locations repeatedly in a short period.
> +   */
> +  nsAutoTArray<nsCOMPtr<nsIURI>, RECENTLY_VISITED_URI_SIZE>
> +    mRecentlyVisitedURIs;

I'm fine if you leave this oneline, even if it's going slightly over 80 chars. It's not a mandatory rule when following it makes readability significantly worse.

btw, since you use this later in isRecentlyVisited, I think you could maybe
typedef nsAutoTArray<nsCOMPtr<nsIURI>, RECENTLY_VISITED_URI_SIZE>
        RecentlyVisitedArray;
and
RecentlyVisitedArray mRecentlyVisitedURIs;
RecentlyVisitedArray::size_type

::: toolkit/components/places/tests/browser/browser_bug680727.js
@@ +34,5 @@
> +
> +      // Go to the offline error page.
> +      content.location = kUniqueURI.spec;
> +    }
> +  );

You seem to do this visited check later in errorListener already, so I'm not sure if there's a point in doing it here... as it is, it may happen at any time between this point and the other check, so doesn't look much useful.

@@ +49,5 @@
> +  window.removeEventListener("DOMContentLoaded", errorListener, false);
> +  ok(Services.io.offline, "Services.io.offline is true.");
> +
> +  // This is an error page.
> +  is(gBrowser.contentDocument.documentURI.substring(0,27),

space after comma

@@ +59,5 @@
> +     "Docshell URI is the original URI.");
> +
> +  // Global history does not record URI of a failed request.
> +  waitForAsyncUpdates(function() {
> +    gAsyncHistory.isURIVisited(kUniqueURI,errorAsyncListener);

space after comma

@@ +104,5 @@
> +}
> +
> +registerCleanupFunction(function() {
> +  Services.io.offline = false;
> +  gBrowser.removeCurrentTab();

Could you please also RemoveEventListener all the listeners you added? if the test will fail this will ensure cleanup, if it succeeds this second remove call will just be a no-op.

@@ +110,5 @@
> +
> +//------------------------------------------------------------------------------
> +// Helper functions:
> +// copied from browser/components/places/tests/browser/head.js
> +function waitForAsyncUpdates(aCallback, aScope, aArguments)

actually you are lucky, if you put this test in browser/components/places/tests/browser/, as you did, head.js is already imported, so you don't need to further copy this function, it's in scope already.
I think the test was in docshell/ before, when I suggested copying it.
Comment 43 Justin Lebar (not reading bugmail) 2011-12-12 10:14:06 PST
> RecentlyVisitedArray::size_type

Can we please use PRUint32?  Tons of code already relies on nsTArray::size_type == PRUint32, and it's much clearer.
Comment 44 Marco Bonardo [::mak] 2011-12-12 10:57:10 PST
(In reply to Justin Lebar [:jlebar] from comment #43)
> Can we please use PRUint32?  Tons of code already relies on
> nsTArray::size_type == PRUint32, and it's much clearer.

I don't see how it's clearer, but I have nothing against it.
Comment 45 O. Atsushi (Torisugari) 2011-12-13 02:49:29 PST
Created attachment 581213 [details] [diff] [review]
Simple proposal v9.1 part 1 (toolkit/components/places)

(In reply to Marco Bonardo [:mak] from comment #44)
> I don't see how it's clearer, but I have nothing against it.

I like PRUint32 too, but I think the reviewer should decide the coding style. Both seems reasonable here. Though I don't like to break the 80-chars rule at any cost... For my poor monitor.

(In reply to Marco Bonardo [:mak] from comment #42)
> @@ +110,5 @@
> > +
> > +//------------------------------------------------------------------------------
> > +// Helper functions:
> > +// copied from browser/components/places/tests/browser/head.js
> > +function waitForAsyncUpdates(aCallback, aScope, aArguments)
> 
> actually you are lucky, if you put this test in
> browser/components/places/tests/browser/, as you did, head.js is already
> imported, so you don't need to further copy this function, it's in scope
> already.
> I think the test was in docshell/ before, when I suggested copying it.

Sadly, it's in <toolkit/components/places/tests/browser/>, not in <browser/components/places/tests/browser/>. Indeed, I had expected that at first.
Comment 46 Marco Bonardo [::mak] 2011-12-13 05:27:39 PST
(In reply to O. Atsushi (Torisugari) from comment #45)
> Sadly, it's in <toolkit/components/places/tests/browser/>, not in
> <browser/components/places/tests/browser/>. Indeed, I had expected that at
> first.

Well, you can add the missing util to toolkit/components/places/tests/browser/heads.js I think they are not in sync just because we didn't add any toolkit browser test recently.

Regarding size_type, there is the fact moving across different architectures PRUint32 may not be the best choice, on 64bit it would be slower. Now, ideally, nsTArray should choose the best size_type based on the architecture, even if I doubt it does nowadays, it may do in future. See also the stdint.h thread in m.d.platform where some of this is discussed.
Comment 47 O. Atsushi (Torisugari) 2011-12-13 06:54:51 PST
Created attachment 581255 [details] [diff] [review]
Simple proposal v9.2 part 1 (toolkit/components/places)

(In reply to Marco Bonardo [:mak] from comment #46)
> Well, you can add the missing util to
> toolkit/components/places/tests/browser/heads.js I think they are not in
> sync just because we didn't add any toolkit browser test recently.

I moved the Copy & Paste portion.

> Regarding size_type, ...

I see. Then size_t will be the better, in the future.
Comment 48 Marco Bonardo [::mak] 2011-12-13 13:40:16 PST
Comment on attachment 581255 [details] [diff] [review]
Simple proposal v9.2 part 1 (toolkit/components/places)

Review of attachment 581255 [details] [diff] [review]:
-----------------------------------------------------------------

It looks ok, thank you!
Comment 49 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-14 14:23:20 PST
Comment on attachment 580707 [details] [diff] [review]
Simple proposal v9 part 2 (docshell)

Olli, punting this to you.  Please let me know if you really want me to do it, but I think you know this code better than I do at this point.
Comment 50 Marco Bonardo [::mak] 2012-02-15 12:10:04 PST
Olli, this is waiting review from 2 months, any ETA on it?
Comment 51 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-26 17:18:16 PST
sorry for the delay. Trying to look at the patch really soon.
(The code is regression risky, and tricky to review)
Comment 52 Marco Bonardo [::mak] 2012-03-21 07:35:22 PDT
review ping (3 months).  May someone else do a first-pass in the meanwhile? Maybe jlebar?
I have other fixes that may touch near code and don't want to bitrot with this ( I actually hope it won't be needed).
Comment 53 Justin Lebar (not reading bugmail) 2012-03-21 07:51:49 PDT
> Regarding size_type, there is the fact moving across different architectures PRUint32 may not be the 
> best choice, on 64bit it would be slower. Now, ideally, nsTArray should choose the best size_type 
> based on the architecture, even if I doubt it does nowadays, it may do in future. See also the 
> stdint.h thread in m.d.platform where some of this is discussed.

Sorry to belabor this point, but the difference we established in that thread is that signed ints are faster than unsigned for things like arr[2 * i], because the compiler can assume that signed never overflows.  We did not establish any difference between 32-bit and 64-bit ints in that thread, at least not that I recall.

I don't care whether we use size_type or PRUint32 here, but I do care that we don't spread misconceptions about what makes for fast code.  :)

I can take a look at this code, but any changes here are scary as heck.
Comment 54 Justin Lebar (not reading bugmail) 2012-03-21 08:55:39 PDT
Comment on attachment 580707 [details] [diff] [review]
Simple proposal v9 part 2 (docshell)

I think this has a good chance of being right, modulo the comment below.  But I'm not comfortable giving r+ here, myself.

More generally, I don't understand why so much code was touched here.  It seems like all we're doing is moving the |if (aAddToGlobalHistory)| branch up one level tweaking the branch condition, and getting rid of some assertions.

Could you accomplish what you're trying to do here with less code?  I smaug would be faster to review if the change was more obviously correct.

>+    // Update session history.
>+    //
>+    // Create SH Entry (mLSHE) or update SH index only if there is a 
>+    // SessionHistory object (mSessionHistory) in the current frame or
>+    // in the root docshell. See bug 89628.
>+    if (rootSH) {
>+        // If this was a history load or a refresh, update the index in SH.
>+        //
>+        // Otherwise, if this is a fresh page getting loaded for the first time
>+        // and this is the rootDocShell, create a session history entry and add
>+        // it to session history.
>+        if (aLoadType & (LOAD_CMD_HISTORY | LOAD_CMD_RELOAD)) {

The original code uses mLoadType here.  Do you know it's the same?

>+            nsCOMPtr<nsISHistoryInternal> shInternal(do_QueryInterface(rootSH));
>+            if (shInternal) {
>+                rootSH->GetIndex(&mPreviousTransIndex);
>+                shInternal->UpdateIndex();
>+                rootSH->GetIndex(&mLoadedTransIndex);
>+#ifdef DEBUG_PAGE_CACHE
>+                printf("Previous index: %d, Loaded index: %d\n\n",
>+                       mPreviousTransIndex, mLoadedTransIndex);
>+#endif
>             }
>-        }
>-    }
>-
>-    // If this was a history load or a refresh,
>-    // update the index in SH. 
>-    if (rootSH && (mLoadType & (LOAD_CMD_HISTORY | LOAD_CMD_RELOAD))) {
>-        nsCOMPtr<nsISHistoryInternal> shInternal(do_QueryInterface(rootSH));
>-        if (shInternal) {
>-            rootSH->GetIndex(&mPreviousTransIndex);
>-            shInternal->UpdateIndex();
>-            rootSH->GetIndex(&mLoadedTransIndex);
>-#ifdef DEBUG_PAGE_CACHE
>-            printf("Previous index: %d, Loaded index: %d\n\n",
>-                   mPreviousTransIndex, mLoadedTransIndex);
>+        } else if (aLoadType != LOAD_BYPASS_HISTORY &&
>+                   aLoadType != LOAD_ERROR_PAGE &&
>+                   !mLSHE &&
>+                   mItemType == typeContent &&
>+                   mURIResultedInDocument) {

But then if you use mLoadType above, you need to get rid of the else and check |!(aLoadType & (LOAD_CMD_HISTORY | LOAD_CMD_RELOAD)| here.
Comment 55 Marco Bonardo [::mak] 2012-03-21 09:01:14 PDT
I suspect changing this code now may make future changes less painful.  Sure it's more changes and I agree review may take time, though nobody pushed to get a review immediately, there has been quite some time and still there's some.  This code will always be painful, till we start making it less.  Excluding the obvious "possibility of regressions" downsize, do you think the small cleanup he made is making things worse to maintain in future, or better?
Comment 56 Justin Lebar (not reading bugmail) 2012-03-21 09:17:57 PDT
I think the "cleanup" doesn't improve or worsen the code quality to any significant degree, and it makes it much more difficult to verify that the patch is correct.

I don't think it's unreasonable for a reviewer to ask for cleanup to be separated from functional changes.  But I'm not even going that far -- I'm just saying that I think it's more likely that smaug will find the time to review this patch if it's simplified.
Comment 57 Marco Bonardo [::mak] 2012-03-21 09:29:46 PDT
(In reply to Justin Lebar [:jlebar] from comment #56)
> I don't think it's unreasonable for a reviewer to ask for cleanup to be
> separated from functional changes.

Sure, I agree.  My fear is just that we end never cleaning up old code cause it's painful and scary, but this way every time we touch it, it keeps being painful and scary.  Though, I mostly care this bug proceeds atm.
Comment 58 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-21 09:32:08 PDT
I have "clean-up docshell" in my todo list, actually next after I get MutationObserver landed
(it is waiting for review).
Comment 59 Marco Bonardo [::mak] 2012-03-21 09:35:19 PDT
Torisugari-san, could you please make a minimal changes patch to docshell and split the cleanup to a separate bug to speed up the review process as requested?
Comment 60 O. Atsushi (Torisugari) 2012-03-21 10:28:39 PDT
Created attachment 608012 [details] [diff] [review]
Simple proposal v10 part 2 (docshell)

This is almost identical to attachment 567435 [details] [diff] [review] (comment #9).

Now my brain is not working in a fine way (2:30 a.m.), so I might have overlooked some comments I should add/modify, but the logic should be correct, hopefully.
Comment 61 O. Atsushi (Torisugari) 2012-03-21 10:35:00 PDT
Created attachment 608015 [details] [diff] [review]
Simple proposal v10.1 part 2 (docshell)

Oops, PR_FALSE -> false
Comment 62 Justin Lebar (not reading bugmail) 2012-03-21 10:45:49 PDT
Comment on attachment 608015 [details] [diff] [review]
Simple proposal v10.1 part 2 (docshell)

+    bool updateGHistory = !(aLoadType == LOAD_BYPASS_HISTORY ||
+                            aLoadType == LOAD_ERROR_PAGE ||
+                            aLoadType & LOAD_CMD_HISTORY);

+    // XXX This log message is almost useless because |updateSHistory|
+    //     and |updateGHistory| are not correct at this point.

Feel free to fix this, or move it.

+        NS_ASSERTION(!updateSHistory,
                      "We shouldn't be updating history for forced reloads!");

s/history/shistory/ ?

I guess I wasn't entirely clear about what is and isn't "cleanup".  I'm totally OK with this patch making changes which are obviously correct; for example, if you wanted to, you could merge

  if (updateGHistory) {
    if (aAddToGlobalHistory) {

into

  if (updataeGHistory && aAddToGlobalHistory),

as you had earlier.  Or you could add (!!rootSH && aAddToGlobalHistory) to the updateGHistory definition.

What was problematic about the last patch I looked at was the moving of code not relevant to the functional change, in a way made it difficult to verify its correctness.

Anyway, I can't vouch for the functional change here (adding to global history more often), but assuming Mak is OK with it, the rest looks fine to me.
Comment 63 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-21 11:01:36 PDT
Comment on attachment 608015 [details] [diff] [review]
Simple proposal v10.1 part 2 (docshell)

In general, please create patches using -U 8 -p
It significantly helps with reviewing
Comment 64 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-21 11:10:03 PDT
Comment on attachment 608015 [details] [diff] [review]
Simple proposal v10.1 part 2 (docshell)

>+                if (aLoadType & LOAD_CMD_RELOAD) {
>+                    // On a reload request, we don't set redirecting flags.
>+                    previousURI = aURI;
>+                } else {
>+                    ExtractLastVisit(aChannel, getter_AddRefs(previousURI),
>+                                    &previousFlags);
Missing space before &

And this needs tests.
Comment 65 O. Atsushi (Torisugari) 2012-03-22 04:02:33 PDT
(In reply to Justin Lebar [:jlebar] from comment #54)
> Comment on attachment 580707 [details] [diff] [review]
> Simple proposal v9 part 2 (docshell)

> The original code uses mLoadType here.  Do you know it's the same?
Yes, as I said in comment #36 and #37. In that sense, what Mak insists is definitely right; That question should be clear enough for both coders and reviewers at a glance, in the first place. i.e. We should stop using either |aLoadType| or |mLoadType| in that function some day. If, in general, reviewers are reading only diff files and are reluctant to read bug comments, we should invent a more useful way to communicate with reviewers. XXX comment can be a candidate, for they have not been ignored so far. And rolling back the patch version would be also helpful as this bug shows.

(In reply to Justin Lebar [:jlebar] from comment #62)
> Comment on attachment 608015 [details] [diff] [review]
> Simple proposal v10.1 part 2 (docshell)
> 
> +    bool updateGHistory = !(aLoadType == LOAD_BYPASS_HISTORY ||
> +                            aLoadType == LOAD_ERROR_PAGE ||
> +                            aLoadType & LOAD_CMD_HISTORY);
> 
> +    // XXX This log message is almost useless because |updateSHistory|
> +    //     and |updateGHistory| are not correct at this point.
> 
> Feel free to fix this, or move it.
I'd rather like to remove it. I guess none of us in the CC list is using this log message, for |updateHistory| has also been incorrect. I'm afraid it will never be able to be useful in the future.

> +        NS_ASSERTION(!updateSHistory,
>                       "We shouldn't be updating history for forced
> reloads!");
> 
> s/history/shistory/ ?
Yes.

> Or you could add (!!rootSH && aAddToGlobalHistory) to
> the updateGHistory definition.
When we don't have |rootSH|, we should not use global history too? And |updateGHistory| exists simply so as to be conservative. Removing |updateGHistory| makes much more sense if reviewers have no difficulty in reading the diff (that's what comment #18 says).

(In reply to Marco Bonardo [:mak] from comment #18)
> ditto, if we can coalesce conditions above, may be easier to check when
> something happens without bouncing up and down

(In reply to Olli Pettay [:smaug] from comment #63)
> In general, please create patches using -U 8 -p
> It significantly helps with reviewing
Well, my .hgrc says it's -U 8 -p, and "-U" and "8" seem working. But surely "-p" is not working for some reason. I'm sorry, but I have no idea how to fix it. And thank you for pointing it out. If it's just me, there should be a way to solve it...

(In reply to Olli Pettay [:smaug] from comment #64)
> And this needs tests.
It's in the other patch. Or do we have anything else to test on docshell side specifically?
Comment 66 O. Atsushi (Torisugari) 2012-03-22 08:00:33 PDT
Created attachment 608333 [details] [diff] [review]
Simple proposal v10.2 part 2 (docshell)

Addressing 

> s/history/shistory/ ?
,

> I guess I wasn't entirely clear about what is and isn't "cleanup".  I'm
> totally OK with this patch making changes which are obviously correct; for
> example, if you wanted to, you could merge
> 
>   if (updateGHistory) {
>     if (aAddToGlobalHistory) {
> 
> into
> 
>   if (updataeGHistory && aAddToGlobalHistory),
> 
> as you had earlier.  Or you could add (!!rootSH && aAddToGlobalHistory) to
> the updateGHistory definition.

and

> Missing space before &
.

Now it seems "-p" is working. I'm really sorry for the inconvenience.

Note You need to log in before you can comment on or make changes to this bug.