Open
Bug 754029
Opened 12 years ago
Updated 2 years ago
Navigating from a new script tag does not add a session history entry
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
REOPENED
mozilla19
People
(Reporter: jruderman, Unassigned)
References
(Blocks 2 open bugs)
Details
(Keywords: testcase)
Attachments
(5 files, 14 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
That's really bizarre. Justin, Olli, any idea?
Comment 2•12 years ago
|
||
No idea, but that's a pretty cool bug.
Comment 3•12 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.
Comment 4•12 years ago
|
||
>- 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•12 years ago
|
||
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•12 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!
Comment 7•12 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.
> 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•12 years ago
|
||
Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17041 on the spec.
Comment 9•12 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
Updated•12 years ago
|
Attachment #623379 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
> Is it possible to set "by user gesture" flag to JS context rather than inScriptTag flag?
nsEventStateManager::IsHandlingUserInput() ?
Comment 11•12 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.
Comment 12•12 years ago
|
||
(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•12 years ago
|
||
This does not test how IsHandlingUserInput() works.
Comment 14•12 years ago
|
||
> 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•12 years ago
|
||
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•12 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.
Comment 17•12 years ago
|
||
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
Comment 18•12 years ago
|
||
> * location.href should not replace SH entry when !(IsHandlingUserInput()).
s/when !(IsHandlingUserInput())./when it is handling user input./
Comment 19•12 years ago
|
||
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
Comment 20•12 years ago
|
||
Updated•12 years ago
|
Attachment #627524 -
Flags: review?(bzbarsky)
Updated•12 years ago
|
Attachment #627525 -
Flags: review?(bzbarsky)
Comment 21•12 years ago
|
||
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•12 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.
Comment 23•12 years ago
|
||
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)
Comment 24•12 years ago
|
||
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•12 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.
Comment 26•12 years ago
|
||
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
Comment 27•12 years ago
|
||
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
Comment 28•12 years ago
|
||
Comment fix about pending print job.
Attachment #634861 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #634960 -
Flags: review?(bzbarsky)
Updated•12 years ago
|
Attachment #634863 -
Flags: review?(bzbarsky)
Comment 29•12 years ago
|
||
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•12 years ago
|
||
I could review this next week. Certainly not this week.
Updated•12 years ago
|
Attachment #634960 -
Flags: review?(bzbarsky) → review?(bugs)
Updated•12 years ago
|
Attachment #634863 -
Flags: review?(bzbarsky) → review?(bugs)
Comment 31•12 years ago
|
||
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•12 years ago
|
Attachment #634863 -
Flags: review?(bugs) → review+
Comment 32•12 years ago
|
||
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•12 years ago
|
||
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•12 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;
Comment 35•12 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.
Comment 36•12 years ago
|
||
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 37•12 years ago
|
||
Comment on attachment 645297 [details] [diff] [review]
Patch v5.2
Ok.
Attachment #645297 -
Flags: review?(bugs) → review+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 38•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
>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()|.
Updated•12 years ago
|
Attachment #646837 -
Flags: review?(bugs)
Comment 41•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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)
Comment 44•12 years ago
|
||
Please link to http://www.whatwg.org/html; that's the spec we implement.
Comment 45•12 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.
Comment 46•12 years ago
|
||
(Webkit's session history is so broken that I don't really care about it :) )
Comment 47•12 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.
Comment 48•12 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)
Comment 49•12 years ago
|
||
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•12 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.
Comment 51•12 years ago
|
||
Jst, you might remember some history of .location. Any opinion here.
Comment 52•12 years ago
|
||
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)
Comment 53•12 years ago
|
||
smaug, what's the next step here?
Comment 54•12 years ago
|
||
Is there something unclear in comment 52?
Comment 55•12 years ago
|
||
(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•12 years ago
|
||
I thought the patch author would ask the review.
Updated•12 years ago
|
Attachment #645297 -
Flags: review?(bzbarsky)
Comment 57•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #646837 -
Flags: review?(bugs)
Comment 58•12 years ago
|
||
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+
Comment 59•12 years ago
|
||
Torisugari - Are you in a position to land this?
Comment 60•12 years ago
|
||
This would be a good time to land, and wait for some time before making it hard to back this out.
Updated•12 years ago
|
Keywords: checkin-needed
Comment 61•12 years ago
|
||
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•12 years ago
|
||
Comment 63•12 years ago
|
||
That patch could use a commit message...
Comment 65•12 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•12 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
Updated•12 years ago
|
Attachment #645297 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #634863 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #646837 -
Attachment is obsolete: true
Comment 67•12 years ago
|
||
Keywords: checkin-needed
Comment 68•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 69•12 years ago
|
||
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•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 70•12 years ago
|
||
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•12 years ago
|
||
Attachment #671031 -
Attachment is obsolete: true
Comment 72•12 years ago
|
||
To treat setTimeout()'s callback within 1sec as user's input.
Comment 73•12 years ago
|
||
I have no idea why I must not call finish() insied of onload(), but this works just fine in my local tree.
Updated•12 years ago
|
Attachment #731848 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #732233 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #731848 -
Flags: review?(bugs) → review+
Comment 74•12 years ago
|
||
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-
Comment 75•12 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?
Comment 76•12 years ago
|
||
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•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: torisugari → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•