Review Request 128480: fix clang plugin highlighting screwup

[Sven Brauch]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128480/
-----------------------------------------------------------

(Updated July 19, 2016, 9:49 p.m.)


Review request for KDevelop, Kevin Funk and Milian Wolff.


Changes
-------

>From a day of real-world testing, I'd say it got much rarer with this patch already, but I still managed to reproduce it once. Here's the other issue I think there is: The URL Parse Lock is released between finishing a parse job and applying the highlighting. So what can happen very very occasionally is that a new job is started after the previous one finished, but before the highlighting is applied. That then resets the tracker, and discards the modification revision reference which is required to translate the old context's highlighting to the current document state.

In this case, I think the quick workaround is just to not apply highlighting, since the new job will do it anyways a few ms later:

    diff --git a/language/highlighting/codehighlighting.cpp b/language/highlighting/codehighlighting.cpp
    index 59871a6..69c479d 100644
    --- a/language/highlighting/codehighlighting.cpp
    +++ b/language/highlighting/codehighlighting.cpp
    @@ -517,6 +517,12 @@ void CodeHighlighting::applyHighlighting(void* _highlighting)
        delete highlighting;
        return;
    }
    +  if(!tracker->holdingRevision(highlighting->m_waitingRevision)) {
    +    qCDebug(LANGUAGE) << "not holding revision" << highlighting->m_waitingRevision << "not applying highlighting;"
    +                      << "probably a new parse job has already updated the context";
    +    delete highlighting;
    +    return;
    +  }
    
    QVector< MovingRange* > oldHighlightedRanges;

Long-term (for 5.1) I think we should discuss how we keep those revision references around. The current way seems quite messy. I think we should just attach them to the TopDUContext; that's where they belong imo. As long as the context is alive, it should be possible to translate its contents to the current version of the document (otherwise, it is quite useless), and vice versa, when the top context goes away, we can discard the revision as well. Thoughts?


Repository: kdevelop


Description
-------

Terrible patch, I know. Sorry. It cost a lot of nerves and time though and I want somebody to look at / try out whether this is a working fix in principle.

In the end, the issue(s) are simple:

1. we need to hold the foreground lock between reading the modification revision and reading the contents. Otherwise they can mismatch.

2. in buildDUChain(), there was

    envFile->setModificationRevision(ModificationRevision::revisionForFile(context->url()));

which is wrong. It sets the modification revision of the parsing environment file to the revision the file has _right now_. The contents, however, were read before in the constructor, and several milliseconds might have passed until this line is reached, with no locks held to prevent anything.
Instead, we have to set the revision to the one which was stored when the tracker was last reset (which happened right after reading the contents, coupled to the contents by the foreground lock).

Actually I'm not sure if the foreground lock is required in this case, because clang for some reason reads the contents in the constructor, which afaik runs in the main thread anyways.


Diffs (updated)
-----

  languages/clang/clangparsejob.cpp 8375eb5 
  languages/clang/duchain/clanghelpers.cpp cea8cd9 

Diff: https://git.reviewboard.kde.org/r/128480/diff/


Testing
-------

Very hard to reproduce. Apply this patch to kdevplatform:

    diff --git a/language/backgroundparser/documentchangetracker.cpp b/language/backgroundparser/documentchangetracker.cpp
    index 294bc14..6f56250 100644
    --- a/language/backgroundparser/documentchangetracker.cpp
    +++ b/language/backgroundparser/documentchangetracker.cpp
    @@ -237,6 +237,11 @@ KDevelop::RangeInRevision DocumentChangeTracker::transformBetweenRevisions(KDeve
    {
        VERIFY_FOREGROUND_LOCKED

    +    if ( !((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)) ) ) {
    +        qWarning() << "invalid transform: from" << fromRevision << "to" << toRevision
    +                   << "but not both revisions held: [from/to]:" << holdingRevision(fromRevision) << holdingRevision(toRevision);
    +    };
    +
        if((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)))
        {
            m_moving->transformCursor(range.start.line, range.start.column, KTextEditor::MovingCursor::MoveOnInsert, fromRevision, toRevision);
    @@ -348,6 +353,8 @@ void DocumentChangeTracker::unlockRevision(qint64 revision)
            m_moving->unlockRevision(revision);
            m_revisionLocks.erase(it);
        }
    +
    +    qDebug() << "** clearing revision" << revision;
    }

    qint64 RevisionLockerAndClearer::revision() const
    
You get _lots_ of those warnings iff you manage to trigger the issue. The best bet seems to be open a relatively large cpp file, and keep removing text and readding it with Ctrl+Z with varying wait times in between (500ms-parse-delay-ish). Do that for a minute or two and it will trigger eventually.

You still get a few of the warning messages sometimes even with the patch applied (but far more without). I think they are from the problem reporter plugin, and I'm not sure if the plugin does something wrong or the parse job.


Thanks,

Sven Brauch

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160719/296908eb/attachment.html>