The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Document Navigation
P2
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.8beta1
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta
Created attachment 170422 [details]
Testcase

Comment 2

12 years ago
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.)
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

12 years ago
Also on Windows.
OS: Linux → All
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.
Attachment #170489 - Flags: superreview?(darin)
Attachment #170489 - Flags: review?(bzbarsky)
Comment on attachment 170489 [details] [diff] [review]
Fix (diff -w).

Note that this is a diff -w
Attachment #170489 - Attachment description: Fix. → Fix (diff -w).
bz, sorry, didn't read your initial comment before I started looking into this.
Yes, ideally the docshell would deal with all of this...
Johnny, do you still want review on that diff?
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.
Created attachment 171371 [details] [diff] [review]
Solution with load flags
Attachment #171371 - Flags: superreview?(jst)
Attachment #171371 - Flags: review?(darin)
Summary: location.replace("#foo") stops document loads → [FIX]location.replace("#foo") stops document loads
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

12 years ago
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
Attachment #171371 - Flags: review?(darin) → review+
> 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 on attachment 171371 [details] [diff] [review]
Solution with load flags

sr=jst
Attachment #171371 - Flags: superreview?(jst) → superreview+
Summary: [FIX]location.replace("#foo") stops document loads → [FIXr]location.replace("#foo") stops document loads
Created attachment 171886 [details] [diff] [review]
Updated to comments
Attachment #171371 - Attachment is obsolete: true
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....
Attachment #170489 - Flags: review?(bzbarsky) → review-
Fixed.  Marking so, but I agree that we should get the second part of jst's
patch in...
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 18

12 years ago
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-
Attachment #170489 - Flags: superreview?(darin)
This apparently caused bug 297122.
Depends on: 297122
*** Bug 236793 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.