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

Categories

(Core :: DOM: Navigation, defect)

19 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 + verified
firefox20 + verified
firefox21 --- verified

People

(Reporter: TheOne, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: regression, reproducible)

Attachments

(5 files, 3 obsolete files)

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?
Blocks: 754029
Attached file Testcase
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?
Assignee: nobody → doug.turner
Keywords: reproducible
Assignee: doug.turner → bugs
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.
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=d32f30ce11a8
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...
Need to back out also Bug 808035 and Bug 765192.
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+
Attached patch v3Splinter 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
Attachment #701369 - Attachment is obsolete: true
Attachment #701843 - Attachment is obsolete: true
Attachment #702216 - Flags: approval-mozilla-beta?
Attachment #702216 - Flags: approval-mozilla-aurora?
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.
Attached patch For betaSplinter Review
[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.
Attachment #702770 - Flags: review?(bzbarsky)
Attachment #702770 - Flags: approval-mozilla-beta?
Attachment #702216 - Flags: approval-mozilla-beta?
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.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla21
Attached file Testcase for Webkit
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.
Attached file Testcase2 (obsolete) —
> 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.