New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using comparator of DomChangeNotifierPlugin when adding a vertex to state flow graph #347

Closed
mehdimir opened this Issue Oct 2, 2013 · 6 comments

Comments

Projects
None yet
3 participants
@mehdimir

mehdimir commented Oct 2, 2013

Using DomChangeNotifierPlugin forces Crawljax to use method isDomChanged instead of the default comparison of dom states in case of dom change.
If isDomChanged resolves to true, then Crawljax should add new dom to the State flow graph.
However, when Crawljax is adding a new vertex to state flow graph, it uses default dom comparison instead of isDomChange method of DomChangeNotifierPlugin (in method equals of StateVertexImpl class).

I believe this is a bug. I would appreciate a quick fix.

@amesbah

This comment has been minimized.

Show comment
Hide comment
@amesbah

amesbah Oct 3, 2013

Member

Yes, I confirm that this is a bug. We should use the same DOM comparison method everywhere, including the state flow graph when adding new states to the graph.

@alexnederlof what would be the best way to do this?

Member

amesbah commented Oct 3, 2013

Yes, I confirm that this is a bug. We should use the same DOM comparison method everywhere, including the state flow graph when adding new states to the graph.

@alexnederlof what would be the best way to do this?

@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Oct 4, 2013

Member

Well the obvious solution is to just inject the plugin there too, and us that instead of the default comparison plugin. @mehdimir are you able to open a pull request? Otherwise it is going to have to wait.

Member

alexnederlof commented Oct 4, 2013

Well the obvious solution is to just inject the plugin there too, and us that instead of the default comparison plugin. @mehdimir are you able to open a pull request? Otherwise it is going to have to wait.

@mehdimir

This comment has been minimized.

Show comment
Hide comment
@mehdimir

mehdimir Oct 4, 2013

I did inject the plugin in the equal method of StateVertexImpl but it does not work. Apprantly there are other places that you are comparing doms which I am not aware of! does it ring any bell?

mehdimir commented Oct 4, 2013

I did inject the plugin in the equal method of StateVertexImpl but it does not work. Apprantly there are other places that you are comparing doms which I am not aware of! does it ring any bell?

@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Oct 14, 2013

Member

The SFG calls add, which relies on the equals function:

Adds the specified vertex to this graph if not already present. More formally, adds the specified vertex, v, to this graph if this graph contains no vertex u such that u.equals(v). If this graph already contains such vertex, the call leaves this graph unchanged and returns false. In combination with the restriction on constructors, this ensures that graphs never contain duplicate vertices.

The equals method for StateVertexImpl is

@Override
    public boolean equals(Object object) {
        if (object instanceof StateVertex) {
            StateVertex that = (StateVertex) object;
            return Objects.equal(this.strippedDom, that.getStrippedDom());
        }
        return false;
    }

So that is indeed a problem. I will open a PR for this.

Member

alexnederlof commented Oct 14, 2013

The SFG calls add, which relies on the equals function:

Adds the specified vertex to this graph if not already present. More formally, adds the specified vertex, v, to this graph if this graph contains no vertex u such that u.equals(v). If this graph already contains such vertex, the call leaves this graph unchanged and returns false. In combination with the restriction on constructors, this ensures that graphs never contain duplicate vertices.

The equals method for StateVertexImpl is

@Override
    public boolean equals(Object object) {
        if (object instanceof StateVertex) {
            StateVertex that = (StateVertex) object;
            return Objects.equal(this.strippedDom, that.getStrippedDom());
        }
        return false;
    }

So that is indeed a problem. I will open a PR for this.

@alexnederlof

This comment has been minimized.

Show comment
Hide comment
@alexnederlof

alexnederlof Oct 14, 2013

Member

Interesting, because the SFG relies on a Set, the equals and hashcode functions of StateVertex determine the behaviour. This prevents an easy fix. I think I'll build a factory for StateVertexes and remove the DomComparison plugin. That way the logic of DOM comparison moves to the equals method, and is thus used properly everywhere. This has a major impact on core, so I have to review this carefully.

Member

alexnederlof commented Oct 14, 2013

Interesting, because the SFG relies on a Set, the equals and hashcode functions of StateVertex determine the behaviour. This prevents an easy fix. I think I'll build a factory for StateVertexes and remove the DomComparison plugin. That way the logic of DOM comparison moves to the equals method, and is thus used properly everywhere. This has a major impact on core, so I have to review this carefully.

@mehdimir

This comment has been minimized.

Show comment
Hide comment
@mehdimir

mehdimir Oct 26, 2013

For the moment I just by-passed the hashcode method by passing a blank string instead of strippedDom filed :

    @Override
    public int hashCode() {
        return Objects.hashCode(" ");
    }

This way, the comparator always bypasses the hash and uses equal method. It makes the comparison very slow but works for me :)

To resolve this issue one might do the same only if the domComparator plugin is on-duty!

For the moment I just by-passed the hashcode method by passing a blank string instead of strippedDom filed :

    @Override
    public int hashCode() {
        return Objects.hashCode(" ");
    }

This way, the comparator always bypasses the hash and uses equal method. It makes the comparison very slow but works for me :)

To resolve this issue one might do the same only if the domComparator plugin is on-duty!

alexnederlof added a commit that referenced this issue Jan 25, 2014

You can now specify a custom StateVertex factory.
The hashcode and equals in the StateVertex are used in the
StateFlowGraph that Crawljax uses. This means that using a custom DOM
comparator has no use, because in the back-end, the hashcode and equals
will determine the behaviour. This commit fixes that by allowing a user
to specify a custom StateVertex factory where you can create your own
StateVertexes with custom hashcode/equals methods.

Fixes #347

alexnederlof added a commit that referenced this issue Feb 2, 2014

You can now specify a custom StateVertex factory.
The hashcode and equals in the StateVertex are used in the
StateFlowGraph that Crawljax uses. This means that using a custom DOM
comparator has no use, because in the back-end, the hashcode and equals
will determine the behaviour. This commit fixes that by allowing a user
to specify a custom StateVertex factory where you can create your own
StateVertexes with custom hashcode/equals methods.

Fixes #347

alexnederlof added a commit that referenced this issue Feb 2, 2014

You can now specify a custom StateVertex factory.
The hashcode and equals in the StateVertex are used in the
StateFlowGraph that Crawljax uses. This means that using a custom DOM
comparator has no use, because in the back-end, the hashcode and equals
will determine the behaviour. This commit fixes that by allowing a user
to specify a custom StateVertex factory where you can create your own
StateVertexes with custom hashcode/equals methods.

Fixes #347

@alexnederlof alexnederlof closed this in #369 Mar 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment