On a DuckDuckGo result page, if you click on a result link while the result page is still loading, the result page is never recorded to the history. STR: 1. Go to: https://duckduckgo.com/ 2. Enter the search term "tst". 3. While the page has not finished loading, click on the first search result. 4. Wait until the that page fully loaded, then click on the "back"-button Expected results: The DuckDuckGo result page should appear. Actual result: the DuckDuckGo homepage appears. Regression window: Last working version: 2012-10-13: http://hg.mozilla.org/mozilla-central/rev/90857937b601 First broken version: 2012-10-14: http://hg.mozilla.org/mozilla-central/rev/57304bbf9c0e

Maybe broken by bug 754029?

Well, probably what's happening is setTimeout() + setting location.href. We can detect this link click is a user's action: > <a href="javascript: location = 'www.example.com';> link </a> But we can't: > <a href="javascript: setTimeout(function(){location = 'www.example.com';}, 100)"> link </a> Right? Well, I'm talking about "nsEventStateManager::IsHandlingUserInput()". I'm not too sure why DuckDuckGo needs setTimeout() and whether or not we should "fix" this... And a happy new year, from GMT+9 area.

I haven't examined DuckDuckGo's code (it's minified), but the result page address appears in the location bar immediately, even before I click a result link, so location.href must have already been set, right?

> I'm not too sure why DuckDuckGo needs setTimeout() and whether or not we should "fix" > this... Hmm. I wonder whether it makes any sense to use the popup control state (which _is_ propagated across short-enough timeouts, iirc) for this?

Links a la search-engine. I don't have enough time this month. PopupControlState sounded to me something awful at first, because the enum's names are "allowed", "abused" etc. But now I guess it will work as expected.

Hey Doug - looks like Torisugari isn't available to resolve this in the short term, and Olli r+'d the patch in bug 754029 (but I know he's busy with B2G right now). Any suggestions on who can take a look?

So we end up replacing because location(.href) is set somewhere under 'message' event listener. nsEventStateManager::IsHandlingUserInput() returns ofc false. the old behavior explicitly replaced only when executing <script> (though that was hackish too).

Preparing back out patch for Bug 754029 + Bug 801357. Anything else is just too risky.

Comment on attachment 701369 [details] [diff] [review] patch r=me But if we think the spec here is not web-compatible, we need to raise a spec issue.

And there are test failures...

Need to back out also Bug 808035 and Bug 765192.

Er, this one https://tbpl.mozilla.org/?tree=Try&rev=5e845a334659

Comment on attachment 701843 [details] [diff] [review] backout more Back outs Bug 754029 - caused this bug by changing .location handling Bug 801357 - removed code which wasn't needed after Bug 754029 Bug 765192 - changed a test so that it relied on the behavior of Bug 754029 Bug 808035 - a regression from Bug 754029 Seems to finally pass b-c tests

Comment on attachment 701843 [details] [diff] [review] backout more r=me

Includes a missing IID change. /me kicks himself [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 754029 User impact if declined: Some pages can be broken (back-forward may not work as expected) Testing completed (on m-c, etc.): Just about to land Risk to taking this patch (and alternatives if risky): Shouldn't be too risky, since this is just backing out the problematic changes Backing out is by far the least risky fix for this. String or UUID changes made by this patch: There is IID change to nsIScriptContext. It was missed in the original patch. No string changes

Comment on attachment 702216 [details] [diff] [review] v3 IID change ok for Aurora, approving. Will let akeybl make the call on beta but wonder if there's a way to do this without IID for beta and then get this proper fix in the follow up release?

I could add a new interface for beta.

Are these backout patches going to be relanded in a near future (with modifications of course)? It seems to be a huge backout...

It isn't that big backout. Changes to C++ code are quite small. Fixes will re-land once someone has time to look at Bug 754029 again and investigate what behavior would be more web-compatible.

[Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 754029 User impact if declined: Some pages can be broken (back-forward may not work as expected) Testing completed (on m-c, etc.): Landed m-c yesterday, Aurora today Risk to taking this patch (and alternatives if risky): Shouldn't be too risky, since this is just backing out the problematic changes Backing out is by far the least risky fix for this. String or UUID changes made by this patch: No changes, since a new beta-only interface is added.

Comment on attachment 702770 [details] [diff] [review] For beta Boris, look for string "_19" and you should see all the beta-specific changes.

Comment on attachment 702770 [details] [diff] [review] For beta r=me

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130123083802 Verified as fixed on Firefox 19.0b3.

Browsers with Webkit seem to treat non-nested setTimeout(...) within 1000 milliseconds as "User Gesture". Just as the source code says: https://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMTimer.cpp#L42 As far as JavaScript redirection is concerned, this behavior is a bit strange but somewhat reasonable; A heavy html page which definitely takes more than 1 sec deletes the session history entry while a light-weight page should preserve its SH entry. And... probably I should look into their DOM event handler's behavior, though I'm assuming that's almost same as Gecko's popupu blocker's.

> And... probably I should look into their DOM event handler's behavior, > though I'm assuming that's almost same as Gecko's popupu blocker's. I don't understand it very well yet, but, as far as I can tell, my assumption is wrong. It seems independent from popup blocker's condition.

Nevermind. That session history entry was preserved by error pages. If the web page redirects to an existing page, Webkit happily deletes session history entry. Then the difference between Gecko (with 754029's patch) and Webkit seems only |setTimeout(...)|'s behavior. And probably that caused THIS bug.

Now https://duckduckgo.com/ works too smoothly for me so it's hard to test whether it's really fixed or not.