Closed Bug 825544 Opened 7 years ago Closed 7 years ago
Clicking on a duckduckgo result link while the page is loading prevents the result page from being recorded
981 bytes, text/html
44.31 KB, patch
|Details | Diff | Splinter Review|
45.07 KB, patch
|Details | Diff | Splinter Review|
1.10 KB, text/html
2.86 KB, text/html
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?
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).
Attachment #701369 - Flags: review?(bzbarsky)
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.
Attachment #701369 - Flags: review?(bzbarsky) → review+
And there are test failures...
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
Attachment #701843 - Flags: review?(bzbarsky)
Comment on attachment 701843 [details] [diff] [review] backout more r=me
Attachment #701843 - Flags: review?(bzbarsky) → review+
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?
Attachment #702216 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I could add a new interface for beta.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #702770 - Flags: review?(bzbarsky) → review+
Attachment #702770 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
> 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.
Attachment #731473 - Attachment is obsolete: true
Now https://duckduckgo.com/ works too smoothly for me so it's hard to test whether it's really fixed or not.
You need to log in before you can comment on or make changes to this bug.