Closed Bug 1570799 Opened 4 months ago Closed 3 months ago

</script> tag disappears when saving SVG file (using "Web Page, Complete")

Categories

(Core :: DOM: Serializers, defect)

68 Branch
Desktop
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: prekgeo, Assigned: longsonr)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [qa-70b-p2])

Attachments

(2 files)

Attached image test.svg

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. Create a file named test.svg with contents:
    <svg xmlns="http://www.w3.org/2000/svg">
    <script>
    before</script>after
    </svg>
  2. Open the file with Firefox.
  3. Press Ctrl-S (save).
  4. Save the file as result.svg.

Actual results:

The saved file is corrupt. Its contents are:
<svg xmlns="http://www.w3.org/2000/svg">
<script>
beforeafter
</svg>

Expected results:

The correct contents would be:
<svg xmlns="http://www.w3.org/2000/svg">
<script>
before</script>after
</svg>
Notice the </script> tag.

I tested this issue in the following Os:

Windows 10 64bit
Windows 7 64bit
Ubuntu 18.04.2 64bit
MacOs 10.14 64bit

Nightly 70.0a1 affected on all Os
Beta 69.0b10 affected on all Os
Release 68.0.1 affected on all Os

Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → Desktop
Component: General → SVG
Product: Firefox → Core

I can confirm this, when using the (default) "Web Page, Complete" save-type (visible in a dropdown in the file-save dialog).

That saving codepath makes us try to serialize the current DOM, and we apparently seem to lose the closing </script> tag in this case for some reason, which produces invalid XML (and hence invalid SVG).

Priority: -- → P3
Summary: </script> tag disappears when saving SVG file → </script> tag disappears when saving SVG file (using "Web Page, Complete")

Is this a regession?

Yeah, it is. I'll track down the range. Current wide range:
2016-01-02 is good
2019-01-02 is bad

Keywords: regression

1-day regression range (the best that mozregression can provide for now):

INFO: Last good revision: 17e47288c2249ac21bd2d0585b19c835984ed32b (2018-02-28)
INFO: First bad revision: 2667f0b010c959940d7a12b4311d54a6abd74ac5 (2018-03-01)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=17e47288c2249ac21bd2d0585b19c835984ed32b&tochange=2667f0b010c959940d7a12b4311d54a6abd74ac5

(I have no idea which commit(s) from that range might be responsible, though. Nothing jumps out from my first few skims.)

A manual backout of bug 1425520 fixes things.

Regressed by: 1425520
Component: SVG → Serializers

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Priority: P3 → --

hsivonen, mind taking a look?

Flags: needinfo?(hsivonen)

lldb) print maybeFixedNode->GetChildCount()
(uint32_t) $5 = 0
(lldb) print aNode->GetChildCount()
(uint32_t) $6 = 1

Which makes the wheels fall off because we then think we don't need an end tag in ElementNeedsSeparateEndTag

There's been enough adverse fallout from bug 1425520 that I wonder if we should undo the fix and work on a better fix from there. Mirko, thoughts?

Flags: needinfo?(hsivonen) → needinfo?(mbrodesser)

generally the rule seems to be, find the comment where bz says "oh dear, this might be a problem" and work back from there.

BTW I've no idea where to start about tests here.

Henri: which other adverse fallout of https://bugzilla.mozilla.org/show_bug.cgi?id=1425520 do you mean? If nothing else was broken by it and the proposed fix is clean, perhaps we should keep it.

Although I've touched related code, I'm still unfamiliar with much of the potentially affected code. So my judgement should be taken with a grain of salt.

Flags: needinfo?(mbrodesser) → needinfo?(hsivonen)

(In reply to Mirko Brodesser from comment #14)

Henri: which other adverse fallout of https://bugzilla.mozilla.org/show_bug.cgi?id=1425520 do you mean?

I was thinking of bug 1433073, but it was regressed by bug 1409951, so I got bugs mixed up.

Flags: needinfo?(hsivonen)
Assignee: nobody → longsonr
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/97e6c7ca3b6b
pass the original element into nsXMLContentSerializer::CheckElementEnd so that we can determine whether it has children properly r=hsivonen
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Whiteboard: [qa-70b-p2]
You need to log in before you can comment on or make changes to this bug.