277224 - [FIXr]location.replace("#foo") stops document loads

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Testcase

Comment 2

[Darin Fisher]

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 [:bzbarsky]]

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!]

Also on Windows.

Comment 5

[Johnny Stenback (:jst)]

Attachment: 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)]

Comment on attachment 170489 [details] [diff] [review]
Fix (diff -w).

Note that this is a diff -w

Comment 7

[Johnny Stenback (:jst)]

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 [:bzbarsky]]

Johnny, do you still want review on that diff?

Comment 9

[Johnny Stenback (:jst)]

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 [:bzbarsky]]

Attachment: Solution with load flags

Comment 11

[Boris Zbarsky [:bzbarsky]]

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]

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 [:bzbarsky]]

> 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)]

Comment on attachment 171371 [details] [diff] [review]
Solution with load flags

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Fixed.  Marking so, but I agree that we should get the second part of jst's
patch in...

Comment 18

[Darin Fisher]

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: [e-mail]]]

This apparently caused bug 297122.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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