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

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[George Prekas]

Attachment: 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.

Comment 1

[Pablo]

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

Comment 2

[Daniel Holbert [:dholbert]]

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).

Comment 3

[Robert Longson [:longsonr]]

Is this a regession?

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Robert Longson [:longsonr]]

A manual backout of bug 1425520 fixes things.

Comment 8

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 9

[Daniel Holbert [:dholbert]]

hsivonen, mind taking a look?

Comment 10

[Robert Longson [:longsonr]]

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

Comment 11

[Henri Sivonen (:hsivonen)]

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?

Comment 12

[Robert Longson [:longsonr]]

Attachment: Bug 1570799 - pass the original element into nsXMLContentSerializer::CheckElementEnd so that we can determine whether it has children properly r=hsivonen

Comment 13

[Robert Longson [:longsonr]]

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.

Comment 14

[Mirko Brodesser -- away, unused old account]

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.

Comment 15

[Henri Sivonen (: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.

Comment 16

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eed4a2fc52560b03c2ab9a543dc56cd0b9d80c64&selectedJob=263738482

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/97e6c7ca3b6b