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

Status: REOPENED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

That's really bizarre.  Justin, Olli, any idea?

Comment 2

[Justin Lebar (not reading bugmail)]

No idea, but that's a pretty cool bug.

Comment 3

[O. Atsushi (Torisugari)]

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.

Comment 4

[O. Atsushi (Torisugari)]

Attachment: 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[O. Atsushi (Torisugari)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[O. Atsushi (Torisugari)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Is it possible to set "by user gesture" flag to JS context rather than inScriptTag flag?

nsEventStateManager::IsHandlingUserInput() ?

Comment 11

[O. Atsushi (Torisugari)]

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.

Comment 12

[O. Atsushi (Torisugari)]

Attachment: 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.

Comment 13

[O. Atsushi (Torisugari)]

Attachment: Test v1 (mochitest)

This does not test how IsHandlingUserInput() works.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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....

Comment 16

[O. Atsushi (Torisugari)]

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.

Comment 17

[O. Atsushi (Torisugari)]

Attachment: 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.

Comment 18

[O. Atsushi (Torisugari)]

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

Comment 19

[O. Atsushi (Torisugari)]

Attachment: 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.

Comment 20

[O. Atsushi (Torisugari)]

Attachment: Test v3 (mochitest)

Comment 21

[Boris Zbarsky [: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?

Comment 22

[O. Atsushi (Torisugari)]

(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.

Comment 23

[O. Atsushi (Torisugari)]

Attachment: Patch v4

Addressing comment #21.

Comment 24

[O. Atsushi (Torisugari)]

Attachment: 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 ...

Comment 25

[O. Atsushi (Torisugari)]

(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.

Comment 26

[O. Atsushi (Torisugari)]

Attachment: 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...

Comment 27

[O. Atsushi (Torisugari)]

Attachment: 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.

Comment 28

[O. Atsushi (Torisugari)]

Attachment: Patch v5.1

Comment fix about pending print job.

Comment 29

[Boris Zbarsky [: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?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

I could review this next week. Certainly not this week.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 33

[O. Atsushi (Torisugari)]

Attachment: 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...

Comment 34

[O. Atsushi (Torisugari)]

(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;

Comment 35

[O. Atsushi (Torisugari)]

(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.

Comment 36

[O. Atsushi (Torisugari)]

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

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 645297 [details] [diff] [review]
Patch v5.2

Ok.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 39

[Matt Brubeck (:mbrubeck)]

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

Comment 40

[O. Atsushi (Torisugari)]

Attachment: 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()|.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

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 43

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 44

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please link to http://www.whatwg.org/html; that's the spec we implement.

Comment 45

[O. Atsushi (Torisugari)]

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.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

(Webkit's session history is so broken that I don't really care about it :) )

Comment 47

[O. Atsushi (Torisugari)]

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 48

[O. Atsushi (Torisugari)]

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

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 50

[O. Atsushi (Torisugari)]

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.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Jst, you might remember some history of .location. Any opinion here.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 53

[Bobby Holley (:bholley)]

smaug, what's the next step here?

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Is there something unclear in comment 52?

Comment 55

[Bobby Holley (:bholley)]

(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?

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

I thought the patch author would ask the review.

Comment 57

[Boris Zbarsky [: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!

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 59

[Bobby Holley (:bholley)]

Torisugari - Are you in a position to land this?

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

This would be a good time to land, and wait for some time before making it hard to back this out.

Comment 61

[O. Atsushi (Torisugari)]

Attachment: 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?

Comment 62

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=42ff6d18fe6d

Comment 63

[Boris Zbarsky [:bzbarsky]]

That patch could use a commit message...

Comment 65

[Mozilla RelEng Bot]

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/[e-mail]-42ff6d18fe6d

Comment 66

[Mozilla RelEng Bot]

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/[e-mail]-42ff6d18fe6d

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf99e483a06

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bf99e483a06

Comment 69

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 70

[Hixie (not reading bugmail)]

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

Comment 71

[O. Atsushi (Torisugari)]

Attachment: Patch (reviewed and unbitrotted)

Comment 72

[O. Atsushi (Torisugari)]

Attachment: For bug 825544

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

Comment 73

[O. Atsushi (Torisugari)]

Attachment: 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.

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 75

[O. Atsushi (Torisugari)]

> 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?

Comment 76

[Bobby Holley (:bholley)]

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.

Comment 77

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.