Open Bug 754029 Opened 8 years ago Updated 6 months ago

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

Categories

(Core :: DOM: Navigation, defect)

x86_64
macOS
defect
Not set

Tracking

()

REOPENED
mozilla19

People

(Reporter: jruderman, Assigned: torisugari)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: testcase)

Attachments

(5 files, 14 obsolete files)

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
Attached file 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.
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.
Attached patch patch v1 (obsolete) — Splinter Review
>-          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.
> 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?
> > 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
Attachment #623379 - Attachment is obsolete: true
> Is it possible to set "by user gesture" flag to JS context rather than inScriptTag flag?

nsEventStateManager::IsHandlingUserInput() ?
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.
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attached patch Test v1 (mochitest) (obsolete) — Splinter Review
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....
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.
Attached patch Test v2 (mochitest) (obsolete) — Splinter Review
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
> * location.href should not replace SH entry when !(IsHandlingUserInput()).
s/when !(IsHandlingUserInput())./when it is handling user input./
Depends on: 606286
Attached patch Patch v3 (obsolete) — Splinter Review
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
Attached patch Test v3 (mochitest) (obsolete) — Splinter Review
Attachment #627524 - Flags: review?(bzbarsky)
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?
(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.
Attached patch Patch v4 (obsolete) — Splinter Review
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)
Attached patch Test v4 (mochitest) (obsolete) — Splinter Review
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 ...
(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.
Attached patch Patch v5 (obsolete) — Splinter Review
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
Attached patch Test v4 (mochitest-chrome) (obsolete) — Splinter Review
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
Attached patch Patch v5.1 (obsolete) — Splinter Review
Comment fix about pending print job.
Attachment #634861 - Attachment is obsolete: true
Attachment #634960 - Flags: review?(bzbarsky)
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+
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.
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...
(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;
(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.
Attached patch Patch v5.2 (obsolete) — Splinter Review
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+
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
Attached patch Fix timeout error (obsolete) — Splinter Review
>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()|.
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.
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 :) )
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.
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...)
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?
Blocks: 796938
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+
Attachment #646837 - Flags: review?(bugs)
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.
Attached patch all-in-one for checkin (obsolete) — Splinter Review
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...
Duplicate of this bug: 513268
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Blocks: 801357
Depends on: 808035
No longer depends on: 808035
Depends on: 810594
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.
Depends on: 808035
No longer depends on: 808035
Depends on: 808035
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
Attachment #671031 - Attachment is obsolete: true
Attached patch For bug 825544Splinter Review
To treat setTimeout()'s callback within 1sec as user's input.
I have no idea why I must not call finish() insied of onload(), but this works just fine in my local tree.
Attachment #731848 - Flags: review?(bugs)
Attachment #732233 - Flags: review?(bugs)
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-
> 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
You need to log in before you can comment on or make changes to this bug.