search

adobe / brackets

An open source code editor for the web, written in JavaScript, HTML and CSS.
http://brackets.io
MIT License
33.25k stars 7.63k forks source link

Editor unresponsive when clicked on commented CSS while live preview is active #9002

Closed liaujianjie closed 10 years ago

liaujianjie commented 10 years ago

In release 43 (OSX Mavericks), the editor becomes unresponsive when you click on a multi-line comment (that comments out a valid class/id) in CSS while having the live preview running. I tried replicating the bug without plugins and it still occurs.

Mark-Simulacrum commented 10 years ago

@RaymondLim Cannot reproduce unresponsiveness on 64bit Ubuntu 14.04. (on master ece9bbf2f204f61d18aadb8260e8f9ffb3c7a1c9).

I do see a 'Unable to load live preview page' dialog; but this happens after the index.html of the Getting Started project has rendered, although the Brackets' console says this (full stack trace below), but I'm not sure what this means - other than that would probably be the file that Brackets' errors on.

GET http://127.0.0.1:9222/json  Inspector.js:266
    getDebuggableWindows Inspector.js:266
    connectToURL Inspector.js:348
    _openInterstitialPage LiveDevelopment.js:1125
    _doLaunchAfterServerReady LiveDevelopment.js:1242
    (anonymous function) LiveDevelopment.js:1343
    j jquery-2.1.0.min.js:2
    k.fireWith jquery-2.1.0.min.js:2
    e.(anonymous function) jquery-2.1.0.min.js:2
    (anonymous function) jquery-2.1.0.min.js:2
    j jquery-2.1.0.min.js:2
    k.fireWith jquery-2.1.0.min.js:2
    e.(anonymous function) jquery-2.1.0.min.js:2
    onSuccess StaticServer.js:124
    (anonymous function) StaticServer.js:149
    j jquery-2.1.0.min.js:2
    k.fireWith jquery-2.1.0.min.js:2
    NodeConnection._receive
marcelgerber commented 10 years ago

@Mark-Simulacrum That's a rather common error message, which is probably not part of the issue. Apparently nobody knows what it means :D See #8611.

marcelgerber commented 10 years ago

@Mark-Simulacrum I found a more reproducible recipe that works for me on Win. Could you try it?

  1. Create the two files i.html and i.css, with the contents:
<html>
    <head>
        <link rel="stylesheet" href="i.css" type="text/css">
    </head>
    <body>
        Some text.
    </body>
</html>
/* body {
    background-color: black;
    color: white;
} */
  1. Start live preview on i.html and wait
  2. Switch to i.css
  3. Remove */ on line 4, then press Backspace again

Result: The editor becomes unresponsive

marcelgerber commented 10 years ago

git bisect points to @RaymondLim's c954fb4462e4fea23e53e0452eedd3a9b54ecdae.

Mark-Simulacrum commented 10 years ago

@MarcelGerber Yes, your test case does allow me to reproduce the hang. Will test the PR branch soon.

peterflynn commented 10 years ago

@Mark-Simulacrum @MarcelGerber Since you both repro'ed this earlier, would one of you mind confirming the fix before we close it? (I tested it a bit during PR review, but this was a nasty enough bug that it never hurts to get some extra eyes on it). Thanks!

marcelgerber commented 10 years ago

@peterflynn @RaymondLim I'm sorry to say, but using the files from above recipe, the bug got even worse. Latest master (03ab969) on Win 8.1

  1. Create the two files i.html and i.css, with the contents:
<html>
    <head>
        <link rel="stylesheet" href="i.css" type="text/css">
    </head>
    <body>
        Some text.
    </body>
</html>
/* body {
    background-color: black;
    color: white;
} */
  1. Start live preview on i.html and wait
  2. Switch to i.css
  3. Click after */

Result: The editor becomes unresponsive

peterflynn commented 10 years ago

Ugh, sorry -- my fault. My last suggestion for simplifying the loop was not quite right. The stopping condition needs to exactly match how TokenUtils decides whether to stop & return false or not.

@RaymondLim thee potential ways to fix that:

  1. Just change the loop to match movePrevToken() properly -- i.e. while (!(ctx.pos.line <= 0 && (ctx.pos.ch <= 0 || ctx.token.start <= 0)))
  2. Same, but wrap that up in a TokenUtils.isAtStart() utility for clarity
  3. Change movePrevToken() to more consistently stop at the start of the stream, instead of stopping one token sooner when it knows that's the last token available. (This seems more maintainable in the long run, but a tad riskier because it would affect other callers... albeit callers that are probably well unit-tested).
peterflynn commented 10 years ago

@RaymondLim I can post a fix using approach 2 above -- just proofed it out.

marcelgerber commented 10 years ago

Looks good now.

JeffryBooher commented 10 years ago

Closing as fixed. @LiauJianJie let us know if you still see problems.