Navigating from a new script tag does not add a session history entry

RESOLVED INACTIVE

Status

()

Core
Document Navigation
RESOLVED INACTIVE
6 years ago
a day ago

People

(Reporter: Jesse Ruderman, Assigned: O. Atsushi (Torisugari))

Tracking

({testcase})

Trunk
mozilla19
x86_64
Mac OS X
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 14 obsolete attachments)

451 bytes, text/html
Details
368 bytes, text/html
Details
27.24 KB, patch
Details | Diff | Splinter Review
2.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
847 bytes, patch
smaug
: review-
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Created attachment 622899 [details]
testcase

1. Load the testcase.
2. Click the first button, labeled "Navigate from new script".
3. Click the back button.

Result: Skipped the testcase when going back in history.

Expected: Arrived back at the testcase. (This is what Google Chrome does, and it's what happens if the "Navigate from existing script" button is clicked instead.)
That's really bizarre.  Justin, Olli, any idea?
No idea, but that's a pretty cool bug.
(Assignee)

Comment 3

6 years ago
This source comment explains everything.

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsLocation.cpp#612
>  /* Check with the scriptContext if it is currently processing a script tag.
>   * If so, this must be a <script> tag with a location.href in it.
>   * we want to do a replace load, in such a situation. 
>   * In other cases, for example if a event handler or a JS timer
>   * had a location.href in it, we want to do a normal load,
>   * so that the new url will be appended to Session History.
>   * This solution is tricky. Hopefully it isn't going to bite
>   * anywhere else. This is part of solution for bug # 39938, 72197
>   * 
>   */

That is, when |location.assign()| is executed in the script tag (i.e. Not in the any DOM event handlers), it works as if it were |location.replace()|.

In my opinion, it should use nsDOMNavigationTiming instead.
(Assignee)

Comment 4

6 years ago
Created attachment 623379 [details] [diff] [review]
patch v1

>-          if (scriptContext->GetProcessingScriptTag()) {

This part seems the only user of nsIScriptContext::GetProcessingScriptTag() in the tree. So we can mark it as deprecated, or simply remove it.
The proposed patch doesn't do what http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-July/027372.html talks about WebKit doing or what we should do.

It does do what the spec says, but the spec is wrong, and we should raise an issue to that effect.

In particular, consider a page that has a button that sets location on click.  The page has a slow-loading ad.  With the proposed patch (and per spec) when the user clicks the button the load will sometimes be a replace load and sometimes not, racing with the loading of the ad.  That's broken.

The right thing to do, probably, is to either check for "Running a <script> _and_ readystate is not complete" or to do a check for user interaction of some sort as described for WebKit in the post quoted above...  And of course get the spec fixed.  I can post about the spec issue if you'd like.
(Assignee)

Comment 6

6 years ago
> The right thing to do, probably, is to either check for "Running a <script>
> _and_ readystate is not complete" or to do a check for user interaction of
> some sort as described for WebKit in the post quoted above...   

I think "Running a <script> _and_ readystate is not complete" is a bit confusing, because onload() handler can embed a new <script> element, just as this bug's testcase does.

Instead, how about checking whether the first "load" event is fired or not, like what Safari's guy suggested? Until docshell fires the first load event and bubbles, nsDOMNavigationTiming::GetLoadEventStart() returns 0 (initial value). Both concept and implementation would be simple.

> And of course get the spec fixed. I can post about the spec issue if
> you'd like.

Please!
> because onload() handler can embed a new <script> element

location.href sets happening during onload are replace loads no matter what already, I thought.  Quite independent of whether there's a <script> involved.

> Instead, how about checking whether the first "load" event is fired or not

I'm not sure what you mean.  Which first "load" event?
(Assignee)

Comment 9

6 years ago
> > because onload() handler can embed a new <script> element
> 
> location.href sets happening during onload are replace loads no matter what
> already, I thought.  Quite independent of whether there's a <script>
> involved.

Aha. I didn't know about onload(). But "onclick() before readystate=complete" will do. For example, this bug's test case with a very large <img> element would be in race condition. Is it possible to set "by user gesture" flag to JS context rather than inScriptTag flag?

> I'm not sure what you mean.  Which first "load" event?

Please forget about it.

> Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17041 on the spec.

Thank you.


By the way, https://bugs.webkit.org/show_bug.cgi?id=42861 says Webkit handles both location.href and HTMLFormElement::submit().
http://persistent.info/webkit/test-cases/redirects/form.html?0
(Assignee)

Updated

6 years ago
Attachment #623379 - Attachment is obsolete: true
> Is it possible to set "by user gesture" flag to JS context rather than inScriptTag flag?

nsEventStateManager::IsHandlingUserInput() ?
(Assignee)

Comment 11

6 years ago
I wrote testcases of JavaScript redirection.
http://torisugari.hostei.com/development/test/nslocation2.html

Generally specking, Webkit works better than Gecko, but it does not seem easy to tell a redirection was caused by user input or not. They replace the SH entry in [form element + createEvent] case, but not in [anchor element + createEvent] case.
(Assignee)

Comment 12

6 years ago
Created attachment 623611 [details] [diff] [review]
patch v2

(In reply to Boris Zbarsky (:bz) from comment #10)
> nsEventStateManager::IsHandlingUserInput() ?

It's too handy. I can hardly understand why it works like a charm...

And it was moved from SetHrefWithBase to SetURI, because there's no reason we should ignore SetHost, SetPath, etc.
(Assignee)

Comment 13

6 years ago
Created attachment 623612 [details] [diff] [review]
Test v1 (mochitest)

This does not test how IsHandlingUserInput() works.
> because there's no reason we should ignore SetHost, SetPath, etc.

Well, other than the fact that the spec says that .href is special.  Keep in mind that this whole "replace" behavior is a compatibility hack...

Will look at the patch later today.
I really think we should write down the desired behavior clearly here.  I can't tell whether the patch does what we want, because we haven't decided what we want....
(Assignee)

Comment 16

6 years ago
The reason of those hacks is the back button a11y. Maybe nsDocShell::LoadURI() should just return silently if !(IsHandlingUserInput()) and (LOAD_CMD_HISTORY) is set, rather than delete the old session history entry.
(Assignee)

Comment 17

6 years ago
Created attachment 624029 [details] [diff] [review]
Test v2 (mochitest)

only comments fix.

> the desired behavior

In my opinion, 

the least requirements:
* location.href should replace SH entry not only on "load" event but also on "DOMContentLoaded".
(per the spec).

* location.href should not replace SH entry when readyState is "complete".
(per the spec, and what this bug is about).


important enhancements for compatibility:
* location.href should not replace SH entry when !(IsHandlingUserInput()).
(per comment#5, and what this bug is about).


nice-to-have enhancements:
* Not only location.href, but also location.assign(), window.open(), form.submit() etc. should replace SHEntry.
(compat Webkit)

* Maybe NPN_GetURL() as well.

* Or fix bug 606286.
(compat UAAG 1.0, section 3.5)
> Provide a configuration so that when the user
> navigates "back" through the user agent history
> to a page with a client-side redirect, the user
> agent does not re-execute the client-side redirect.
Attachment #623612 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
> * location.href should not replace SH entry when !(IsHandlingUserInput()).
s/when !(IsHandlingUserInput())./when it is handling user input./
(Assignee)

Updated

6 years ago
Depends on: 606286
(Assignee)

Comment 19

6 years ago
Created attachment 627524 [details] [diff] [review]
Patch v3

Addressing comment #14

Drop the hack for location.assign(), along with location.host, location.path, ... etc. Now the target is only location.href.
Attachment #623611 - Attachment is obsolete: true
Attachment #624029 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 627525 [details] [diff] [review]
Test v3 (mochitest)
(Assignee)

Updated

6 years ago
Attachment #627524 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #627525 - Flags: review?(bzbarsky)
Comment on attachment 627524 [details] [diff] [review]
Patch v3

> nsCOMPtr<nsIDocument> document(do_GetInterface(mDocShell));

As far as I can tell, this should always return null (since mDocShell is an nsWeakPtr).  Is this actually returning non-null for you? How?
(Assignee)

Comment 22

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #21)
> > nsCOMPtr<nsIDocument> document(do_GetInterface(mDocShell));
> 
> As far as I can tell, this should always return null (since mDocShell is an
> nsWeakPtr).  Is this actually returning non-null for you? How?

What you point out are always correct; it returns null for me. The problem is it passed my mochitest when I posted the patch, and it is still reproducible.
(Assignee)

Comment 23

6 years ago
Created attachment 629569 [details] [diff] [review]
Patch v4

Addressing comment #21.
Attachment #627524 - Attachment is obsolete: true
Attachment #627525 - Attachment is obsolete: true
Attachment #627524 - Flags: review?(bzbarsky)
Attachment #627525 - Flags: review?(bzbarsky)
(Assignee)

Comment 24

6 years ago
Created attachment 629570 [details] [diff] [review]
Test v4 (mochitest)

Adding more setTimeout and "load" handler.

With this test, the previous patch (Patch v3) successfully fails. i.e. one of the problems was race condition, as is often the case with. Maybe another problem would be subframe's session history handling on parent loading, though I don't want to touch that code / spec ...
(Assignee)

Comment 25

6 years ago
(In reply to O. Atsushi (Torisugari) from comment #24)
> With this test, the previous patch (Patch v3) successfully fails. i.e. one
> of the problems was race condition, as is often the case with.
This seems wrong. It was just failing due to history length saturation (50?) as a result of many retries. 

The actual reason was subframe's busy flag.
>                     if (parentBusy & BUSY_FLAGS_BUSY ||
>                         selfBusy & BUSY_FLAGS_BUSY) {
>                         loadType = LOAD_NORMAL_REPLACE;
>                         shEntry = nsnull; 
>                     }

By the way, bug 673467:
(In reply to Justin Lebar [:jlebar] from comment #1)
> It could be a bug in the site -- maybe they run different script when they
> detect FF 4 or later -- but the site works properly in Chrome, which likely
> also has whatever browser feature they might check for in FF4.
> 
> The issue appears to be with child shentries.

If I understand right, the reason why Google Chrome (or Safari) didn't hit bug 673467 was it replaces all the sh entry: not only child sh entry but also top-frame sh entry, as long as it is loading a document. They are way greedy.

And I'm afraid the "busy or not" may cause the problem which bz said in comment #5
> In particular, consider a page that has a button that sets location on
> click.  The page has a slow-loading ad.  With the proposed patch (and per
> spec) when the user clicks the button the load will sometimes be a replace
> load and sometimes not, racing with the loading of the ad.  That's broken.

, when the button is in a subframe.

Anyway, what I should do would be to use chrome-mochitest instead of normal mochitest so as to avoid subframe discussion.
(Assignee)

Comment 26

6 years ago
Created attachment 634861 [details] [diff] [review]
Patch v5

If nsIDocShell::isExecutingOnLoadHandler's primary consumer is nsLocation, probably we should stop using it and introduce a new flag for

> > 13. Queue a task to mark the Document as completely loaded.

to docshell/document/contentviewer or elsewhere, namely, nsDocShell::mCompletelyLoaded. Well, we call everything "complete" here and there...
Attachment #629569 - Attachment is obsolete: true
(Assignee)

Comment 27

6 years ago
Created attachment 634863 [details] [diff] [review]
Test v4 (mochitest-chrome)

By the way, without applying the patch, test4 and test5 fails.

>+   * Test 4: Load a JS redirecting page; setTimeout(...)'s callback
>+   *         is inserting a new script element into the document. And the
>+   *         inserted script contains |location.href = ...|.

>+   * Test 5: Setting location.href in onDOMContentLoaded() should REPLACE.

Failing test4 is fair enough, that's what this bug is about. But failing test5 seems another bug.
Attachment #629570 - Attachment is obsolete: true
(Assignee)

Comment 28

6 years ago
Created attachment 634960 [details] [diff] [review]
Patch v5.1

Comment fix about pending print job.
Attachment #634861 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #634960 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #634863 - Flags: review?(bzbarsky)
I don't know when I'll be able to get to this, but probably not this week.

Justin, Olli, is either one of you interested?
I could review this next week. Certainly not this week.
Attachment #634960 - Flags: review?(bzbarsky) → review?(bugs)
Attachment #634863 - Flags: review?(bzbarsky) → review?(bugs)
Comment on attachment 634960 [details] [diff] [review]
Patch v5.1


>+      // I expect that nsIDocShell::isExecutingOnLoadHandler is true while
>+      // the document is handling "load", "pageshow", "popstate",
>+      // "readystatechange" for "complete" and pending print() including
>+      // "beforeprint"/"afterprint".
Don't expect, but be sure. So, test and tweak the comment.
(I don't know what popstate has to do with this all)


>+      //
>+      // Maybe this API property needs a better name.
>+      if (!replace) {
>+        docShell->GetIsExecutingOnLoadHandler(&replace);
>+      }
>+
>+      // XXX What about "9. ApplicationCache"?
>+    }
>+  }
please test this, and file a followup if needed.
Attachment #634960 - Flags: review?(bugs) → review+

Updated

6 years ago
Attachment #634863 - Flags: review?(bugs) → review+
As always with session history changes, this is *very* regression risky. So, be prepared to back out.
But, I like the new behavior.

Please file a followup to remove GetProcessingScriptTag() if it isn't used anywhere anymore.
(Assignee)

Comment 33

6 years ago
Created attachment 644752 [details]
Manual test for "afterprint" (Don't click here, if you don't want to lose some pieces of paper)

Since "beforeprint"/"afterprint" handler can't cancel the print job, it seems impossible to write an automated testcase. Though modern OS (Win/Lin/Mac) are able to output a print image file (e.g. PDF) instead of queuing a print job...
(Assignee)

Comment 34

6 years ago
(In reply to Olli Pettay [:smaug] from comment #31)
> >+      // I expect that nsIDocShell::isExecutingOnLoadHandler is true while
> >+      // the document is handling "load", "pageshow", "popstate",
> >+      // "readystatechange" for "complete" and pending print() including
> >+      // "beforeprint"/"afterprint".
> Don't expect, but be sure. So, test and tweak the comment.
> (I don't know what popstate has to do with this all)

The testcase covers "load", "pageshow" and "readystate". So what's uncertain to me was only "popstate". But that's my mistake. "popstate" on loading process was gone around Firefox 4.0. 
http://hacks.mozilla.org/2011/03/history-api-changes-in-firefox-4/

Except for a comment in nsDocShell::EndPageLoad(),
>    // Notify the ContentViewer that the Document has finished loading.  This
>    // will cause any OnLoad(...) and PopState(...) handlers to fire.
>    if (!mEODForCurrentDocument && mContentViewer) {
>        mIsExecutingOnLoadHandler = true;
>        mContentViewer->LoadComplete(aStatus);
>        mIsExecutingOnLoadHandler = false;
(Assignee)

Comment 35

6 years ago
(In reply to Olli Pettay [:smaug] from comment #31)
> >+      // XXX What about "9. ApplicationCache"?
> please test this, and file a followup if needed.

I'm at a loss what to test. According to the spec, the document should queue "pending application cache download process tasks", between "pageload" event and "beforeprint" event.

That is. from
http://hg.mozilla.org/mozilla-central/annotate/82b6c5885345/layout/base/nsDocumentViewer.cpp#l1036
to
http://hg.mozilla.org/mozilla-central/annotate/82b6c5885345/layout/base/nsDocumentViewer.cpp#l1059

Since it does not seem to handle ApplicationCache there, maybe that is a bug. But I'm not too sure it is worth fixing. If the document does not invoke AppCache event ("checking" and possibly "downloading") handlers *synchronously*, there's no chance it sets |location.href| anyway.

(In reply to Olli Pettay [:smaug] from comment #32)
> Please file a followup to remove GetProcessingScriptTag() if it isn't used
> anywhere anymore.

I will, if this lands successfully.
(Assignee)

Comment 36

6 years ago
Created attachment 645297 [details] [diff] [review]
Patch v5.2

Only comments, as described above, besides

>-    // will cause any OnLoad(...) and PopState(...) handlers to fire.
>+    // will cause any OnLoad(...) handlers to fire.

@nsDocshell.cpp
Attachment #634960 - Attachment is obsolete: true
Attachment #645297 - Flags: review?(bugs)
Comment on attachment 645297 [details] [diff] [review]
Patch v5.2

Ok.
Attachment #645297 - Flags: review?(bugs) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/551b6acdafb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0b0dd6dfa2

Don't forget commit messages in your patches, please!
Assignee: nobody → torisugari
Flags: in-testsuite+
Keywords: checkin-needed
Sorry, I backed this out on inbound because it looks like it may have caused timeouts in several tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec566153ef8
https://tbpl.mozilla.org/php/getParsedLog.php?id=13921849&tree=Mozilla-Inbound
(Assignee)

Comment 40

6 years ago
Created attachment 646837 [details] [diff] [review]
Fix timeout error

>ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug311007.xul | Test timed out.
>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tab_dragdrop.js | Test timed out
>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tab_dragdrop.js | Found a browser window after previous test timed out
>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tabfocus.js | Test timed out
>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tabfocus.js | Found a tab after previous test timed out: data:text/html,<html%20id='tab2'><body><button%20id='button2'>Tab%202</button></body></html>
>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tabfocus.js | Found a tab after previous test timed out: data:text/html,<html%20id='tab3'><body><button%20id='button3'>Tab%203</button></body></html>

These tests wrongly expect that setting |location.href| leaves the previous SHEntry. Using |location.assign()| fixes them.

>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_593003_iframe_wrong_hud.js | Timed out while waiting for: iframe network request displayed in tab1
>TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_chrome.js | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHistory.replaceState] at chrome://mozapps/content/extensions/extensions.js:185

It does not seem to me that this file (browser_webconsole_chrome.js) use session history.

However,
> function addTab(aURL)
> {
>   gBrowser.selectedTab = gBrowser.addTab();
>-  content.location = aURL;
>+  content.location.assign(aURL);
>   tab = gBrowser.selectedTab;
>   browser = gBrowser.getBrowserForTab(tab);
> }
fixes the problem like a charm, though I don't want to count up how many times other webconsole tests call |addTab()|.
(Assignee)

Updated

6 years ago
Attachment #646837 - Flags: review?(bugs)
Comment on attachment 646837 [details] [diff] [review]
Fix timeout error

Hmm, this is worrisome, like changing tests always is.
I need to re-review the original patch.
Torisugari, could you test how other browsers work in these cases?
Is .location = foo different from location.assign(foo).

Especially test IE. It has the least broken session history in general.
Comment on attachment 646837 [details] [diff] [review]
Fix timeout error

Ask review again once you have done some more testing.

(Sorry, I'm rather strict here, but changes to session history handling etc
are risky.)
Attachment #646837 - Flags: review?(bugs)
Please link to http://www.whatwg.org/html; that's the spec we implement.
(Assignee)

Comment 45

6 years ago
I wrote 3 tests;

1. inline <script> tag.
http://torisugari.hostei.com/development/test/location_href+inline_script.html

2. on "DOMContentLoaded" event.
http://torisugari.hostei.com/development/test/location_href+domcontentloaded.html

3. on "load" event.
http://torisugari.hostei.com/development/test/location_href+load.html

And the result of Firefox 14 is
1: replace
2: not replace
3: replace

This is a little odd behavior, because the order of invoking scripts is 1->2->3. This was pointed out at WebKit's bugzilla in 2010, btw.

https://bugs.webkit.org/show_bug.cgi?id=42861#c10
> Basically, it looks like Firefox doesn't create a new history
> entry if the JS redirect happens inline, or if it's during an
> onload (but not DOMContentLoaded) handler, ...

And their behavior now is
1: replace
2: replace
3: replace

My IE9 on Windows7 (32bit) is
1: not replace
2: not replace
3: not replace

So basically IE9 is not compatible with Firefox. IE9's onDOMContentLoaded's behavior happens to be same as Firefox's, but IE has been supporting "DOMContentLoaded" only since IE9. I don't think the compatibility is more important than that of WebKit.

As for timeout in Mozilla's automated tests, I originally wrote "test_bug311007.xul" and you reviewed it. The point where my test script was wrong was nsIWebProgressListener::onLocationChange() (+setTimeout(0)), which is, iirc, synchronous to DOMContentLoaded(). In other words, the script depended on the buggy behavior. Though I agree that 4 timeouts are too many and surely are surprising,..

Oh, and I don't understand where webconsole's test calls |history.back()| or |history.go()|, yet. Maybe my assumption is not correct at all.
(Webkit's session history is so broken that I don't really care about it :) )
(Assignee)

Comment 47

6 years ago
As far as content redirection's session history is concerned, Webkit is way too greedy than whatwg's spec requires. But their behavior is more or less inspired by Gecko, so as to improve accessibility of "back" button. Or maybe so as to improve inaccessibility of intermediate HTML files.

Anyway, IE9's is different from Mozilla1.x - Firefox 14's. But I believe it's not impossible to improve a11y without hacking session history like this.
(Assignee)

Comment 48

6 years ago
Comment on attachment 646837 [details] [diff] [review]
Fix timeout error

In summary,

       locaion.href  location.assign()

IE9    not replace   not replace
WebKit replace       replace
Gecko  replace       replace
whatwg replace       (implicitly not replace)
patch  replace       not replace

The reason why Gecko decided to replace the SH entry would have been a11y. Indeed, IE's back button is broken since its response speed is too nice. On the other hand, as long as bug 606286 is fixed, a11y is not a big problem for Gecko. So we should replace SH entries not because of accessibility but simply because of backwards compatibility, in addition to evangelism. In other words, I don't mind Firefox behaves like IE (and "fix" whatwg's spec).
Attachment #646837 - Flags: review?(bugs)
Wouldn't it be regression risky to change behavior which is at least somewhat compatible with
one other browser engine to behavior which is only compatible with the spec.


(Trying to decide what I think we should do with this...)
(Assignee)

Comment 50

6 years ago
I think few, if any, websites actually use |assign()| on loading. And few of them would expect |assign()| works as if it were |replace()|, for they can use |replace()| when their web developers really want to replace SH.

Besides, Gecko does NOT replace on setting |location.host|. Nor |protocol|,  |hostname|, |port|, |pathname|, |search|, |hash|. Since the spec never refers to them, Gecko's behaviour is absolutely correct. But it is not compatible with Webkit, which always replaces everything on loading.

Trident [replace]     : replace()
        [not replace] : assign(), href, protocol, host, ...

Gecko [replace]       : replace(), href, assign()
      [not replace]   : protocol, host, port, ...

Webkit [replace]      : replace(), assign(), href, protocol, host, ...
       [not replace]  : (none)

whatwg [replace]      : replace(), href
       [not replace]  : (probably) assign(), protocol, host, port, ... 

I'm afraid moving |assign()| from [replace] group to [not replace] is not a significant change, in terms of compatibility to other browsers.


On the other hand, things about |loacation.href| is really regression risky, and indeed we came across the timeout errors. But that is a bug.
Jst, you might remember some history of .location. Any opinion here.
Comment on attachment 646837 [details] [diff] [review]
Fix timeout error

I can't quite decide what I think about this all, so would be good to get review 
for the real patch also from bz and after that I could review these tests.
Attachment #646837 - Flags: review?(bugs)
smaug, what's the next step here?
Is there something unclear in comment 52?
(In reply to Olli Pettay [:smaug] from comment #54)
> Is there something unclear in comment 52?

Yes. The comment suggests that bz should review the patch, but you didn't flag him for review, which is why I wasn't sure whether there was something that needed to be done in the interim. Should he be flagged?
I thought the patch author would ask the review.
Attachment #645297 - Flags: review?(bzbarsky)
Comment on attachment 645297 [details] [diff] [review]
Patch v5.2

s/by the time/before/ in the comment

r=me with that.  Thank you for sorting through the compat story here, and writing the excellent comments and even better patch!
Attachment #645297 - Flags: review?(bzbarsky) → review+
Comment on attachment 646837 [details] [diff] [review]
Fix timeout error

This patch isn't scary, but is it a bit worrisome that these changes are needed.
Attachment #646837 - Flags: review?(bugs) → review+
Torisugari - Are you in a position to land this?
This would be a good time to land, and wait for some time before making it hard to back this out.
Keywords: checkin-needed
(Assignee)

Comment 61

6 years ago
Created attachment 671031 [details] [diff] [review]
all-in-one for checkin

Thank you very much.

This is s/nsnull/nullptr/ and s/nsCAutoString/nsAutoCString/ version.

Now my concern is, another "timeout" may have been added to the tree, so far. Could anybody test this patch on the try server first?
That patch could use a commit message...
(Assignee)

Updated

6 years ago
Duplicate of this bug: 513268

Comment 65

6 years ago
Try run for 42ff6d18fe6d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=42ff6d18fe6d
Results (out of 232 total builds):
    success: 212
    warnings: 17
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-42ff6d18fe6d

Comment 66

6 years ago
Try run for 42ff6d18fe6d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=42ff6d18fe6d
Results (out of 239 total builds):
    success: 217
    warnings: 19
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-42ff6d18fe6d
Attachment #645297 - Attachment is obsolete: true
Attachment #634863 - Attachment is obsolete: true
Attachment #646837 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6bf99e483a06
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Updated

6 years ago
Blocks: 801357

Updated

6 years ago
Depends on: 808035
(Assignee)

Updated

6 years ago
No longer depends on: 808035

Updated

6 years ago
Depends on: 810594

Updated

6 years ago
Depends on: 818153
I may back this out to fix Bug 825544 (and if so, need to back out also Bug 801357).
But I haven't investigated Bug 825544 yet.

Updated

5 years ago
Depends on: 808035

Updated

5 years ago
No longer depends on: 808035

Updated

5 years ago
Depends on: 808035

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
O. Atsushi (Torisugari): If you could describe what you think the spec should say, and what led you to that conclusion, in the WHATWG bug, that would be fantastic. https://www.w3.org/Bugs/Public/show_bug.cgi?id=17041
(Assignee)

Comment 71

5 years ago
Created attachment 731846 [details] [diff] [review]
Patch (reviewed and unbitrotted)
Attachment #671031 - Attachment is obsolete: true
(Assignee)

Comment 72

5 years ago
Created attachment 731848 [details] [diff] [review]
For bug 825544

To treat setTimeout()'s callback within 1sec as user's input.
(Assignee)

Comment 73

5 years ago
Created attachment 732233 [details] [diff] [review]
Surpress ASSERTION like bug 854195

I have no idea why I must not call finish() insied of onload(), but this works just fine in my local tree.
(Assignee)

Updated

5 years ago
Attachment #731848 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #732233 - Flags: review?(bugs)

Updated

5 years ago
Attachment #731848 - Flags: review?(bugs) → review+
Comment on attachment 732233 [details] [diff] [review]
Surpress ASSERTION like bug 854195

So what, do your patches end up causing the assertion without this change?
In case of session history doing stuff within load event listener is
quite different from doing it afterwards, so I'm not ready to
randomly change this change.
Attachment #732233 - Flags: review?(bugs) → review-
(Assignee)

Comment 75

5 years ago
> So what, do your patches end up causing the assertion without this change?

Exactly. I guess it was introduced 1-2 months ago. Besides bug 825544, bug 854195 and bug 846154 show the same error. In my opinion, that range-check is reasonable. So probably there's something strange in docshell's session history handling, instantly. Anyway, it does not mean that there's something wrong with these unit-tests.

> In case of session history doing stuff within load event listener is
> quite different from doing it afterwards, so I'm not ready to
> randomly change this change.

You are right, but what should I do? This test can land?
IMO, we shouldn't hold new test coverage hostage to assertions that are known to fail in other tests. If we don't have the resources to fix the existing culprits, I don't think it's fair to ask Torisugari to tackle the problem. We still have the failure annotation here, so we can dig into it whenever we have time.
No longer blocks: 796938

Comment 77

a day ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: REOPENED → RESOLVED
Last Resolved: 6 years agoa day ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.