Drag&Drop imports wrongly gradients (leads to crash)(rev >= 13139)

Bug #1318657 reported by FirasH
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Fix Released
High
David Mathog

Bug Description

Inkscape is not able to manage some gradients imported using drag&drop from file manager.

- Steps to reproduce:
1) Download the attached file
2) Open Inkscape
3) From your file manager drag&drop the .svg in Inkscape

Sidebar fills are missing, they're shown as transparent

4) Select one of the wrongly imported gradients (transparent) and open: Object > Fill and Stroke

Inkscape crashes

- System information:
openSUSE 13.1 x86_64, Inkscape 0.48+devel (r13358)

Revision history for this message
FirasH (firashanife) wrote :
description: updated
su_v (suv-lp)
tags: added: crash gradient importing regression
Revision history for this message
su_v (suv-lp) wrote :

Reproduced with current trunk r13358 on OS X 10.7.5.

Based on tests with archived builds, the crash in Fill&Stroke seems to be triggered by the changes in revision 13139
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13139>

Rev <= 13044: all three gradients import ok
Rev >= 13047: the gradients of the two objects left and right incorrectly share the gradient definition and position of the center object
Rev >= 13139: the gradients of the two objects left and right refer to a gradient definition which is omitted on import (linearGradient4100-8)

Changed in inkscape:
importance: Undecided → High
milestone: none → 0.91
status: New → Confirmed
summary: - Drag&Drop imports wrongly gradients (leads to crash)
+ Drag&Drop imports wrongly gradients (leads to crash)(rev >= 13139)
Revision history for this message
David Mathog (mathog) wrote :

The problem can be replicated by

1. Start inkscape
2. Open the test file.
3. new
4. select contents from first window, paste into second window.

The trigger for this problem is that the initial SVG has two exactly identical gradients named linearGradient4100 and linearGradient4100-8. Rename all references to 4100-8 to 4100, remove the unneeded 4100-8 in defs, and the file will work correctly when cut and paste. (Attached.) In the original test file, it doesn't matter what order the two gradients are copied over, 4100 first or last, the program will crash. Looking into it further now.

Revision history for this message
David Mathog (mathog) wrote :

I will not have time to get back to this for a week. I'm posting what I have in case somebody else wants to have a look in the meantime.

The attached patch (may be applied to rev 13372) does NOT fix the issue, and I don't see why. The problem appeared to be that there was a missing case, it would only fix references for REF_STYLE and this was an instance of REF_HREF. So the appropriate code was moved in from another routine - and it did what it was supposed to, according to DEBUG statements in this patch, but it didn't make any difference with respect to the final bug. Strange.

The next attachment is the test case to use with that patch. To see the problem, do this:

1. start inkscape
2. file->open gradients-on-import-5.svg
3. file->new->default
4. select the little piece in the first window, copy it to the second
5. select the bigger piece in the first window, copy it to the second

The little piece uses linearGradient4100-8, and the bigger one is linearGradient4100, and these are identical. Since 4100-8 came in first all references to 4100 should be changed to 4100-8, and 4100 itself should be dropped. The DEBUG statements show that the xlink:href in lineargradient18282 is changed to use the former one. But by the time it gets to the point where the XML editor may be employed it is back to linearGradient4100. The other part of the code still worked, dropping 4100, so xlink:href to a nonexistent lineargradient, and BOOM.

I guess either the operation is working on a copy, and so is ignored, or something later, elsewhere, overwrites the change.

Here are the DEBUG statements that come out at step 5:
DEBUG importDefs 1
DEBUG prevent_id_clashes
DEBUG find_references xlink:href val #linearGradient4100
DEBUG change_def_references before find_references looking for linearGradient4100 to change to linearGradient4100-8
DEBUG find_references xlink:href val #linearGradient4100
DEBUG change_def_references after find_references
DEBUG change_def_references found refernces to linearGradient4100
DEBUG change_def_references for loop type 0
DEBUG change_def_references REF_HREF new_uri #linearGradient4100-8 of attribute xlink:href
DEBUG change_def_references changed element linearGradient18282 attribute xlink:href new value #linearGradient4100-8
DEBUG importDefs linearGradient18282 xlink:href is #linearGradient4100-8

Revision history for this message
David Mathog (mathog) wrote :

This is the test case mentioned in my prior post.

su_v (suv-lp)
Changed in inkscape:
status: Confirmed → Triaged
Revision history for this message
David Mathog (mathog) wrote :

Much debugging later...

What is happening is apparently that change_def_references() in id-clash.cpp is setting the xlink:href value in the document's XML tree but for some reason or other the corresponding SPObject representation is not updated to match. The code in change_def_references() uses

                it->elem->getRepr()->setAttribute(it->attr, new_uri);

to set the value in the XML and that is working as it should. But coming out of importDefs() in document.cpp the corresponding SPObject still (and forever) has the old definition (like it was before the setAttribute) and that causes the rest of the problems. It may be that the XML is later written over by the SPObject form of the data - I have not debugged it that far.

Some flag needs to be set somewhere to force the SPObjects to be recreated from the XML??? Or is it necessary to set both the SPObject and its corresponding XML at the same time? Note that the code in question is pretty much identical to that in fix_up_refs() in id-clash.cpp, and that seems not to have the same problem.

Revision history for this message
David Mathog (mathog) wrote :

Note, the preceding refers to what happens when the patch two messages up is applied.

Revision history for this message
David Mathog (mathog) wrote :

OK, I finally see what the problem is. For future reference the issue is this:

1. in the SVG <defs> there are two records with id's linearGradient4100 and linearGradient18282 *IN THAT ORDER*
2. linearGradient18282 has an xlink:href to linearGradient4100
3. at some point during import the order of these records is REVERSED
4. within importDefs() in one cycle linearGradient18282 is encountered and is copied from the clipboard to the document
5. in a later cycle linearGradient4100 is encountered and found to be identical to another record already in <defs>,
consequently all references to linearGradient4100 are changed to that preexisting record *IN THE CLIPBOARD XML*

Which is too late, because the record where it matters has already been transferred to the document. A classic case of shutting the barn door after the horse has left.

Now to see if I can fix this without too much trauma. Perhaps one full pass through the clipboard to resolve all of the duplicates and then a second pass to copy over the new ones.

Revision history for this message
David Mathog (mathog) wrote :

The logic to handle this is getting more and more complex. Consider this scenario:

1. paste clipboard into a document
2. paste it into the same document a second time
3. paste it into a new document

Current processing marks up the XML in the clipboard. If at (1) there is a gradient "B" similar to a gradient "A"
in the document, then all references to "B" are changed to "A" and the gradient "B" is marked for omission.
That is fine at (2). However it all goes wrong at (3) because the new document does not have "A", yet all of the references
to "B" have been removed, and "B" itself has been marked as a duplicate!

So it looks like to do this safely one must make a 2nd copy of the clipboard on every paste operation, mark it up to remove duplicates / change references, and then throw it away.

There are also problems not only with the clipboard containing duplicates of definitions in the document, but containing duplicates of definitions within itself. When this logic is working right Inkscape will not make these, but there is nothing to prevent other applications from creating such an SVG, like the example for this bug.

Revision history for this message
David Mathog (mathog) wrote :

I think the applied patch finally resolves this issue. Please test - I don't want to commit it until at least one other developer has tried to break it.

The issue described in the previous post turned out not to be a problem because something upstream was already making a duplicate of the clipboard and passing that in.

With the current code an SVG that contains duplicated gradients/swatches in defs will have these reduced to a single instance if the diagram is cut/paste or if it is imported. However, they will still both be there if the file is just "opened", as that does not go through "importDefs()" which is the routine that cleans these up. Also, any definitions that should have been in "defs", but were not, will not receive any special processing. So if one of those is a duplicate, it will pass through as a duplicate.

Tested with revision 13400 on linux only.

Revision history for this message
Johan Engelen (johanengelen) wrote :

Comments about the patch in #10:

- Please reformat the code.
      if(defid.find(DuplicateDefString) != Glib::ustring::npos)break;
  is very hard to read.

- Could you change the "if (it->type == REF_HREF)" list to a switch statement?

- This section
+ SPObject *trg = source->getObjectByRepr(laterDef);
+ Glib::ustring newid = trg->getId();
+ if(newid.find(DuplicateDefString) != Glib::ustring::npos)continue; // this one already handled
+ if (trg && SP_IS_GRADIENT(trg) && src != trg) {
  crashes if trg == nullptr.
  Also, please don't rely on operator precedence and put parens around src!=trg

- About
+ if (gr->isEquivalent(SP_GRADIENT(trg)) &&
+ gr->isAligned(SP_GRADIENT(trg))) {
   SP_GRADIENT(trg) is potentially expensive, so don't duplicate it unnecessarily.

- About
+ gboolean duplicate = false;
  Use C++ bool instead.

Revision history for this message
David Mathog (mathog) wrote :

Thanks. I did not convert to a switch statement, as it is one of those instances where {} are needed inside most of the case statements, and it ended up being longer and uglier than the if/else structure. Made changes to reduce the number of SP_GRADIENT() uses and the other "active" code changes listed above. The attached patch also includes the patch for 1298967 since that affects the same sections of code, and the changes for 1298967 are just a couple of lines. I also rolled in changes so that copy/paste of an object with a gradient created by inkscape does not duplicate the gradient as it had previously.

To see the new gradient behavior:

1. create a blue rectangle, black border
2. in fill/stroke convert it to a linear gradient
3. select it and paste

there will NOT be another gradient type - both objects will reference the same type.

4. select the new rectangle and start the gradient tool.
5. drag the handles to rotate and scale the gradient.

there will still NOT be another gradient type. This is because the gradients listed under fill/stroke is the one at the end of the chain, and inkscape creates an intervening lineargradient record. The intervening one was modified by the handle changes, but the one at the end wasn't.

Here is where it gets a little odd..

6. Copy/paste the second rectangle.
7. With the third rectangle still selected change it to a radial gradient.

There is STILL no new gradient. What has happened is that inkscape has changed the intervening gradient from linear->radial, but the gradient at the end of the chain, which is listed, is still the same.

Things that DO create new gradient types are changes to the colors at the handles.

Additionally, if the entire lineargradient description is in a single record, which is the case when gradients are read in from EMF files, then when objects filled with those gradients are cut/paste new gradients are created. This is because the gradienttransform in those cases is in the single lineargradient, not in the intervening one, and if the transform changes a new gradient type is required.

If an end user wants to create a duplicate of an existing gradient type they can do so by starting with a solid rectangle (for instance), and then converting it to a linear gradient. That will place a new gradient on the list, even when it is identical to an existing one. However, if a file containing the duplicate is imported or cut/paste to another window then that duplicate will be eliminated in the target window.

I am going to commit these changes after I'm done posting here.

Revision history for this message
David Mathog (mathog) wrote :

committed as revision 13417.

Changed in inkscape:
assignee: nobody → David Mathog (mathog)
status: Triaged → Fix Committed
Revision history for this message
David Mathog (mathog) wrote :

Other notes.

1. Because the "end" gradient which is listed in the Fill/Stroke dialog is not being unnecessarily duplicated, and a large number of objects are pointing to it, if ONE of those objects is converted to a swatch in the Fill/Stroke dialog, then ALL the objects referring to that gradient will see it as a swatch. (The conversion can be undone with a ^Z.)

2. If one of the objects pointing to a single gradient is converted to a pattern, and then later converted back to a gradient, a new gradient is created from the current default color. In some instances it looks like it is reverting to the gradient it had before it was changed to a pattern, but that isn't what is happening.

3. It may be that there will now be instances where an end user would want to duplicate a gradient in the fill/stroke gradient list and change its name. I don't know if there is currently a way of doing this, since it was never needed before.

Revision history for this message
Johan Engelen (johanengelen) wrote :

Thanks for the changes.

I made additional changes in rev. 13420.

" I did not convert to a switch statement, as it is one of those instances where {} are needed inside most of the case statements, and it ended up being longer and uglier than the if/else structure."
You should always use {} inside all case statements. I made the change for you and I find it much cleaner than the if/else structure. As a bonus, we get compile-time checks that all type cases are handled.

Also look at the other changes I made. You are new'ing too much (we are not programming in Java! ;).

Please study my changes in rev. 13420.

Revision history for this message
David Mathog (mathog) wrote :

Re: 22

Many of the lines that you changed were not originally committed by me. (I don't think my patch added a single "new".)

Bryce Harrington (bryce)
Changed in inkscape:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.