Last Comment Bug 277224 - [FIXr]location.replace("#foo") stops document loads
: [FIXr]location.replace("#foo") stops document loads
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 All
: P2 normal (vote)
: mozilla1.8beta1
Assigned To: Boris Zbarsky [:bz]
: Adam Lock
Mentors:
: 236793 (view as bug list)
Depends on: 297122
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-05 20:19 PST by Boris Zbarsky [:bz]
Modified: 2006-03-12 18:14 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (159 bytes, text/html)
2005-01-05 20:20 PST, Boris Zbarsky [:bz]
no flags Details
Fix (diff -w). (5.40 KB, patch)
2005-01-06 13:13 PST, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review-
Details | Diff | Splinter Review
Solution with load flags (20.32 KB, patch)
2005-01-15 14:19 PST, Boris Zbarsky [:bz]
darin.moz: review+
jst: superreview+
Details | Diff | Splinter Review
Updated to comments (12.42 KB, patch)
2005-01-20 08:54 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2005-01-05 20:19:21 PST
This is akin to bug 233963, but for location.replace().  The same solution as
there will not work, since we want different load flags.

So I'm left wondering -- why is the Stop() call in nsLocation anyway?  Wouldn't
it be better to put it in docshell, which already does Stop() inside
InternalLoad(), and just have a way for the caller to indicate that Stop()
should get the STOP_CONTENT flag?  That way the hack from bug 233963 could be
removed, too.
Comment 1 Boris Zbarsky [:bz] 2005-01-05 20:20:17 PST
Created attachment 170422 [details]
Testcase
Comment 2 Darin Fisher 2005-01-05 20:29:30 PST
Stop has interesting side-effects.  Do we need to worry about the ordering of
events at all?  If Stop it delayed until InternalLoad could we be changing the
order of the side-effects in any way?  (I sort of suspect not; your plan sounds
like a good one.)
Comment 3 Boris Zbarsky [:bz] 2005-01-05 20:38:52 PST
The basic idea is to not Stop() when we're just doing an anchor scroll.  Since
InternalLoad() would be getting called sync from the code in question, I suspect
it's fine.  The Stop() is just there to avoid us parsing any more data (which
may have come in from the network already) after the location has been changed.
 For that purpose, it's ok as long as it happens within the location setter
somewhere.
Comment 4 OstGote! 2005-01-06 02:02:24 PST
Also on Windows.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-06 13:13:37 PST
Created attachment 170489 [details] [diff] [review]
Fix (diff -w).

This fixes this bug by using nsIURL::GetRetRelativeSpec() to check the
difference from the current URI and the URI we're loading, and if we're loading
a ref, we won't STOP_CONTENT at all.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-06 13:14:48 PST
Comment on attachment 170489 [details] [diff] [review]
Fix (diff -w).

Note that this is a diff -w
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-06 14:15:31 PST
bz, sorry, didn't read your initial comment before I started looking into this.
Yes, ideally the docshell would deal with all of this...
Comment 8 Boris Zbarsky [:bz] 2005-01-06 21:20:29 PST
Johnny, do you still want review on that diff?
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-07 12:12:14 PST
Hmm, I guess I don't care, though the changes to nsStandardURL still apply (even
if not needed for this change per se). So yeah, but only the nsStandardURL part.
Comment 10 Boris Zbarsky [:bz] 2005-01-15 14:19:00 PST
Created attachment 171371 [details] [diff] [review]
Solution with load flags
Comment 11 Boris Zbarsky [:bz] 2005-01-15 14:26:27 PST
Comment on attachment 170489 [details] [diff] [review]
Fix (diff -w).

>Index: netwerk/base/src/nsStandardURL.cpp

>+    if (((*thatIndex == '#') && !*thisIndex) || foundHash ||

So that first condition is "this has no anchor and the other does", "foundHash"
is "both have anchors"....

>+        ((thatIndex != startCharPos) && (*(thatIndex - 1) == '#'))) {

What case does this condition correspond to?  Document it, please (as well as
the other two)?
Comment 12 Darin Fisher 2005-01-15 15:23:43 PST
Comment on attachment 171371 [details] [diff] [review]
Solution with load flags

>Index: docshell/base/nsIWebNavigation.idl

>+ /*
>+  * XXXbz note that these flags are generally mutually exclusive; the two
>+  * exceptions are that one can specify both LOAD_FLAGS_BYPASS_CACHE and
>+  * LOAD_FLAGS_BYPASS_PROXY together and one can specify both
>+  * LOAD_FLAGS_REPLACE_HISTORY and LOAD_FLAGS_STOP_CONTENT together.
>+  */   
>+ /*
>+  * XXXbz some of these flags only apply to reload(), but that's not
>+  * clearly documented
>+  */

Ack.. see my patch for bug 99625.  I really need to polish off that patch!
I'd rather you leave out these comments so that there is less to conflict
with when I try to update the other (which I plan to do once that POPUP
load flag is sorted out).


>  /**
>+  * Flag to force stopping of content if the load ends up happening
>+  * (default is to only stop network activity).
>+  */
>+  const unsigned long LOAD_FLAGS_STOP_CONTENT    = 0x0800;

Might be good to mention what stopping content means here or at least
to refer to the STOP_CONTENT flag.  Also, why is the default to only
stop network activity?	Is that because we want to keep animating 
images during page transition?	(BTW, I assume the content is stopped
once the load that "ends up happening" is handled by this webnav, and
not before, right?


>Index: docshell/base/nsDocShell.cpp

>+        } else if (LOAD_TYPE_HAS_FLAGS(aLoadType, LOAD_FLAGS_STOP_CONTENT)) {
>+            rv = Stop(nsIWebNavigation::STOP_CONTENT);
>         } else {
>             rv = Stop(nsIWebNavigation::STOP_NETWORK);

Does STOP_CONTENT also imply STOP_NETWORK?  Don't we need to stop both
content and network?


r=darin
Comment 13 Boris Zbarsky [:bz] 2005-01-16 09:47:08 PST
> Ack.. see my patch for bug 99625.

OK.  I'll take these changes out, but could you roll something equivalent into
your patch?

> Might be good to mention what stopping content means here or at least
> to refer to the STOP_CONTENT flag.

Good point.  Will do that.

>  Also, why is the default to only stop network activity? Is that because we
> want to keep animating images during page transition?

I guess so... Not actually sure.  ;)  The checkin comments aren't very helpful here.

> (BTW, I assume the content is stopped once the load that "ends up happening"
> is handled by this webnav, and not before, right?

Nope.  The Stop() call is made right before we try to NS_NewChannel.  The main
reason for this is that we want to stop before we add the new channel to our
loadgroup; stopping after would stop the new channel as well.  A vicious cycle,
but I'm not sure how to get around it with the existing loadgroup setup and api....

> Does STOP_CONTENT also imply STOP_NETWORK? 

Good catch.  It does not.  I'll switch to STOP_ALL and combine this condition
with the |zombieViewer| condition.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-18 16:30:58 PST
Comment on attachment 171371 [details] [diff] [review]
Solution with load flags

sr=jst
Comment 15 Boris Zbarsky [:bz] 2005-01-20 08:54:56 PST
Created attachment 171886 [details] [diff] [review]
Updated to comments
Comment 16 Boris Zbarsky [:bz] 2005-01-20 11:03:27 PST
Comment on attachment 170489 [details] [diff] [review]
Fix (diff -w).

Marking r- per comment 11.  I don't see why the code I ask about there is
correct....
Comment 17 Boris Zbarsky [:bz] 2005-01-20 11:03:41 PST
Fixed.  Marking so, but I agree that we should get the second part of jst's
patch in...
Comment 18 Darin Fisher 2005-01-23 11:05:58 PST
Comment on attachment 170489 [details] [diff] [review]
Fix (diff -w).

assuming you don't want me to review this since it has bz's r-
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-06-20 08:53:52 PDT
This apparently caused bug 297122.
Comment 20 Boris Zbarsky [:bz] 2005-08-28 13:48:57 PDT
*** Bug 236793 has been marked as a duplicate of this bug. ***

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