Closed Bug 975707 Opened 6 years ago Closed 6 years ago

Links in Console load in Console's view on double-click, breaking Console till reopen (if the second click hits the link, not a pop-up)

Categories

(DevTools :: Console, defect, major)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: Aleksej, Assigned: sjakthol)

Details

(Whiteboard: [bugday-20140309])

Attachments

(1 file, 1 obsolete file)

2014-02-22-03-02-04-mozilla-central-firefox-30.0a1.en-US.linux-x86_64

Steps to reproduce
1. Open Developer Tools → Console.  Optionally disable all buttons but "Net".
2. Load a page which would cause non-short links appear in the Console (e.g. http://ya.ru).  
3. Double-click a link, making sure the second click goes on the link (if a pop-up window appears and interferes, choose the right side of a long link instead).

Actual results:
The link is loaded in the Console view (frame or window), the console is unusable until DevTools are closed and reopened.
I can reproduce it. No need to have long links, just make sure that the mouse location is not above the opened popup.

Mihai, should this be a secure bug ?
Severity: normal → major
Flags: needinfo?(mihai.sucan)
OS: Linux → All
Hardware: x86_64 → All
A patch that fixes this problem by also stopping double click events on message links from propagating to the default handler.

I'm a but unsure about the test case as it uses setTimeout to wait 500ms for navigation before testing if the frame navigated. I don't know if there's a better way to test for a lack of event or location change but if there is, I'll improve the test case.
Attachment #8380301 - Flags: review?(mihai.sucan)
Comment on attachment 8380301 [details] [diff] [review]
webconsole-stop-double-click-on-link-from-propagating.patch

Review of attachment 8380301 [details] [diff] [review]:
-----------------------------------------------------------------

This is a really good patch. Thank you Sami! Also, thanks to Aleksej for the bug report.

Sami: the test fails here. See https://pastebin.mozilla.org/4379427 I only get this failure when I run all of the webconsole tests. If I run only the test, it passes.

::: browser/devtools/webconsole/test/browser.ini
@@ +225,5 @@
>  [browser_webconsole_bug_817834_add_edited_input_to_history.js]
>  [browser_webconsole_bug_821877_csp_errors.js]
>  [browser_webconsole_bug_837351_securityerrors.js]
>  [browser_webconsole_bug_846918_hsts_invalid-headers.js]
> +[browser_webconsole_bug_975707_dont_navigate_on_doubleclick.js]

Please dont include the bug number in the file name. (we are deprecating bug numbers in file names)

::: browser/devtools/webconsole/test/browser_webconsole_bug_975707_dont_navigate_on_doubleclick.js
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You should use the public domain license boilerplate from https://www.mozilla.org/MPL/headers/ (for tests)

@@ +21,5 @@
> +    let messages = yield waitForMessages({
> +      webconsole: hud,
> +      messages: [{
> +        name: "Network request message",
> +        url: TEST_URI

Please also add category: CATEGORY_NETWORK to make sure this is matching what we want.

@@ +32,5 @@
> +    is(hud.iframeWindow.location, CONSOLE_FRAME_URI, "Location before double click is CONSOLE_FRAME_URI.");
> +    EventUtils.synthesizeMouseAtCenter(urlNode, {clickCount: 2}, hud.iframeWindow);
> +
> +    let deferred = promise.defer();
> +    setTimeout(deferred.resolve, 500);

Instead of the timeout you could:

- wait for the click event and check if preventDefault() was called (event.defaultPrevented).
- wait for domcontentloaded/load (check which one is fired when your fix is not applied). in the event handler you can do ok(false, "console iframe window navigated").
Attachment #8380301 - Flags: review?(mihai.sucan)
(In reply to Girish Sharma [:Optimizer] from comment #1)
> I can reproduce it. No need to have long links, just make sure that the
> mouse location is not above the opened popup.
> 
> Mihai, should this be a secure bug ?

Not sure. Does this allow privilege escalation?
Flags: needinfo?(mihai.sucan)
(In reply to Mihai Sucan [:msucan] from comment #4)
> (In reply to Girish Sharma [:Optimizer] from comment #1)
> > I can reproduce it. No need to have long links, just make sure that the
> > mouse location is not above the opened popup.
> > 
> > Mihai, should this be a secure bug ?
> 
> Not sure. Does this allow privilege escalation?

Not sure either :)
Thanks for the review. Here's a new version that should fix the issues you pointed out.

For the test I chose to check that once click event arrives, the default action is prevented. Apparently when all tests were run the page is cached and thus doesn't show up in the console at all. This was causing the test to fail earlier...
Attachment #8380301 - Attachment is obsolete: true
Attachment #8380628 - Flags: review?(mihai.sucan)
Comment on attachment 8380628 [details] [diff] [review]
webconsole-stop-double-click-on-link-from-propagating-v2.patch

Review of attachment 8380628 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for the quick update. Tests pass now.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=189cb0884245
Attachment #8380628 - Flags: review?(mihai.sucan) → review+
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Try push seems green enough.

Landed: https://hg.mozilla.org/integration/fx-team/rev/70f928fe9ecf
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/70f928fe9ecf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Verified fixed with 2014-03-09-03-02-04-mozilla-central-firefox-30.0a1.en-US.linux-x86_64: double click causes selection of the word.  Thanks.
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20140309]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.