Uncaught error over multiple paste (or other action) over an image #1578

jodator · 2019-02-25T11:45:19Z

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

latest master (with this PR but probably unrelated)

📋 Steps to reproduce

Copy some large content with images (ie from this doc page)
Go to some editor example (I've used baloon editor build)
Paste the content at the begining
Paste the content again over an image
Repeat process (1-3 times)

✅ Expected result

Should work.

❎ Actual result

Error is thrown:

Most of the times:

remove.js:16 Uncaught TypeError: Cannot read property 'parentNode' of undefined
   at Qs (remove.js:16)
   at Xs._updateChildren (renderer.js:557)
   at Xs.render (renderer.js:203)
   at Fr._render (view.js:673)
   at Fr.on (view.js:182)
   at Fr.fire (emittermixin.js:204)
   at Fr.change (view.js:475)
   at Fr._disableRendering (view.js:660)
   at Kl.listenTo (editingcontroller.js:82)
   at Kl.fire (emittermixin.js:204)

Sometimes:

domconverter.js:138 Uncaught TypeError: Invalid value used as weak map key
   at WeakMap.set (<anonymous>)
   at so.bindElements (domconverter.js:138)
   at Xs._updateElementMappings (renderer.js:302)
   at Xs._updateChildrenMappings (renderer.js:280)
   at Xs.render (renderer.js:176)
   at Fr._render (view.js:673)
   at Fr.on (view.js:182)
   at Fr.fire (emittermixin.js:204)
   at Fr.change (view.js:475)
   at Fr._disableRendering (view.js:660)

📃 Other details that might be useful

It's easier to spot this error with larger content. Ie pasting an image with 3-4 paragraphs might not be enough to cause it.

Spotted on Chrome.

The text was updated successfully, but these errors were encountered:

Mgsy · 2019-02-25T11:50:58Z

I can confirm this issue.

jodator · 2019-02-25T11:51:04Z

ps.: Does not happen on docs page https://ckeditor.com/docs/ckeditor5/latest/examples/builds/balloon-editor.html

Reinmar · 2019-02-25T12:16:06Z

Can you paste here simplified HTML which causes this issue? The page you used may be changed soon and we may not be able to reproduce this issue yet.

ps.: Does not happen on docs page https://ckeditor.com/docs/ckeditor5/latest/examples/builds/balloon-editor.html

Does it mean it's a regression?

jodator · 2019-02-25T13:23:07Z

Sooooo the minimal set of content

Here is a very long snippet with 200 p elements and one image inside a figure

	<p>00</p>
	<figure class="image image-style-side" contenteditable="false">
		<img src="sample.jpg" alt="sample image">
		<figcaption>image</figcaption>
	</figure>

	<p>01</p>
	<p>02</p>
	<p>03</p>
	<p>04</p>
	<p>05</p>
	<p>06</p>
	<p>07</p>
	<p>08</p>
	<p>09</p>

	<p>10</p>
	<p>11</p>
	<p>12</p>
	<p>13</p>
	<p>14</p>
	<p>15</p>
	<p>16</p>
	<p>17</p>
	<p>18</p>
	<p>19</p>

	<p>20</p>
	<p>21</p>
	<p>22</p>
	<p>23</p>
	<p>24</p>
	<p>25</p>
	<p>26</p>
	<p>27</p>
	<p>28</p>
	<p>29</p>

	<p>30</p>
	<p>31</p>
	<p>32</p>
	<p>33</p>
	<p>34</p>
	<p>35</p>
	<p>36</p>
	<p>37</p>
	<p>38</p>
	<p>39</p>

	<p>40</p>
	<p>41</p>
	<p>42</p>
	<p>43</p>
	<p>44</p>
	<p>45</p>
	<p>46</p>
	<p>47</p>
	<p>48</p>
	<p>49</p>

	<p>50</p>
	<p>51</p>
	<p>52</p>
	<p>53</p>
	<p>54</p>
	<p>55</p>
	<p>56</p>
	<p>57</p>
	<p>58</p>
	<p>59</p>

	<p>60</p>
	<p>61</p>
	<p>62</p>
	<p>63</p>
	<p>64</p>
	<p>65</p>
	<p>66</p>
	<p>67</p>
	<p>68</p>
	<p>69</p>

	<p>70</p>
	<p>71</p>
	<p>72</p>
	<p>73</p>
	<p>74</p>
	<p>75</p>
	<p>76</p>
	<p>77</p>
	<p>78</p>
	<p>79</p>

	<p>80</p>
	<p>81</p>
	<p>82</p>
	<p>83</p>
	<p>84</p>
	<p>85</p>
	<p>86</p>
	<p>87</p>
	<p>88</p>
	<p>89</p>

	<p>90</p>
	<p>91</p>
	<p>92</p>
	<p>93</p>
	<p>94</p>
	<p>95</p>
	<p>96</p>
	<p>97</p>
	<p>98</p>
	<p>99</p>

	<p>101</p>
	<p>102</p>
	<p>103</p>
	<p>104</p>
	<p>105</p>
	<p>106</p>
	<p>107</p>
	<p>108</p>
	<p>109</p>

	<p>110</p>
	<p>111</p>
	<p>112</p>
	<p>113</p>
	<p>114</p>
	<p>115</p>
	<p>116</p>
	<p>117</p>
	<p>118</p>
	<p>119</p>

	<p>120</p>
	<p>121</p>
	<p>122</p>
	<p>123</p>
	<p>124</p>
	<p>125</p>
	<p>126</p>
	<p>127</p>
	<p>128</p>
	<p>129</p>

	<p>130</p>
	<p>131</p>
	<p>132</p>
	<p>133</p>
	<p>134</p>
	<p>135</p>
	<p>136</p>
	<p>137</p>
	<p>138</p>
	<p>139</p>

	<p>140</p>
	<p>141</p>
	<p>142</p>
	<p>143</p>
	<p>144</p>
	<p>145</p>
	<p>146</p>
	<p>147</p>
	<p>148</p>
	<p>149</p>

	<p>150</p>
	<p>151</p>
	<p>152</p>
	<p>153</p>
	<p>154</p>
	<p>155</p>
	<p>156</p>
	<p>157</p>
	<p>158</p>
	<p>159</p>

	<p>160</p>
	<p>161</p>
	<p>162</p>
	<p>163</p>
	<p>164</p>
	<p>165</p>
	<p>166</p>
	<p>167</p>
	<p>168</p>
	<p>169</p>

	<p>170</p>
	<p>171</p>
	<p>172</p>
	<p>173</p>
	<p>174</p>
	<p>175</p>
	<p>176</p>
	<p>177</p>
	<p>178</p>
	<p>179</p>

	<p>180</p>
	<p>181</p>
	<p>182</p>
	<p>183</p>
	<p>184</p>
	<p>185</p>
	<p>186</p>
	<p>187</p>
	<p>188</p>
	<p>189</p>

	<p>190</p>
	<p>191</p>
	<p>192</p>
	<p>193</p>
	<p>194</p>
	<p>195</p>
	<p>196</p>
	<p>197</p>
	<p>198</p>
	<p>199</p>

Load editor with 200+ nodes in root.
Select an image.
type, paste, whatever - it will crash
Select the image.

jodator · 2019-02-25T15:01:23Z

Simplified case:

Load content with 150+ (ie 150 <p>) and one <image> in root. (the minimal nodes required for fastDiff - so total nodes number before and after modification must be over 300)
Select image
Hit delete

jodator · 2019-02-25T16:21:39Z

OK so the problem was the fake selection container (FSC) in the DOM. The fastDiff() method returned different results then normal diff if the last element in the DOM was a FSC.

The FSC is not a part of a view that diff should care about. It has own logic for rendering in the renderer. Previously (i might be wrong here) the diff method marked FSC from the DOM as removed element or it was removing it and other rendering algorithms didn't bother with it. The fastDiff created different results when the FSC was in the DOM hence the renderer tried to rended unexisting node (or do something with it).

I have a fix for but I only need to add tests to it.

The solution is to remove the FSC before running any diff methods.

Reinmar · 2019-02-25T21:39:12Z

I'm testing with this content:

	let d = '<figure class="image"><img alt="bar" src="sample.jpg"></figure>';

	for ( let i = 0; i < 200; ++i ) {
		d += `<p>${ i }</p>`;
	}

	editor.setData( d );

The TC is:

Click the image.
Delete it.

What I see in updateChildren() is that there are 200 insert operations and 202 delete operations.

If this method was responsible for rendering the FSC, then it'd be a valid result. HOWEVER, FSC is being rendered by other methods. It's not one of the "children" to be rendered. And so, it should not be used when diffing or mapping children. At least that's how it was meant to be when we were implementing it because I'm not sure what happens now.

Reinmar · 2019-02-25T22:19:08Z

This is baaaaad. FSC's being re-rendered by the renderer like there's no tomorrow. It seems that every time root children are being modified, the FSC is first being removed by the updateChildren() method and then added back by renderFakeSelection().

These are results of quoting and unquoting the image several times:

The image is selected for the entire period and there should be no need to touch FSC. And yet we constantly add and remove it.

Now, interestingly, I cannot easily show that removing FSC for a brief moment will cause any issues. However, I will insist on being careful with it. I can imagine browsers doing weird things if you suddenly remove the element in which the selection was placed. It will vary based on what's around that element and whether this is a mobile/desktop browser (e.g. iOS has more tendency to scroll to the selection). It also doesn't mean anything that it works this way today because those things are still not speced. Furthermore, there are screen readers which are going to read the selection changes.

Finally, there's ckeditor/ckeditor5-engine#1610 which I now understand. It was completely unclear to me because updateChildren() wasn't supposed to touch FSC :D Why would we have to unbind it in updateChildren()? Now it's clear. cc @kobsy

Therefore, we should find a way to skip FSC when comparing nodes. Fortunately, it seems that there are only two places where we use "actual DOM children" and we can (hopefully) ignore this element there. Curious if this is going to work...

Reinmar · 2019-02-25T22:34:30Z

I created a quick PoC in ckeditor/ckeditor5-engine#1687. It seems to fix both issues (both errors that were logged) and all tests pass.

Reinmar · 2019-02-25T22:36:31Z

Also, that's how the log looks now when quoting/unquoting an image:

Mgsy · 2019-02-26T10:17:21Z

I was testing the solution from ckeditor/ckeditor5-engine#1687 and I was able to reproduce the same error as in the first post. It occurs also on master.

Steps to reproduce

Open the article sample.
Copy the whole content.
Put the caret at the end of the UL List item 2.
Hold Ctrl/Cmd + V (editor should hang for a while).
Keep pressing Ctrl/Cmd + V repeatetly (it should be quick).

Error

remove.js:16 Uncaught TypeError: Cannot read property 'parentNode' of undefined
    at remove (remove.js:16)
    at Renderer._updateChildren (renderer.js:557)
    at Renderer.render (renderer.js:203)
    at View._render (view.js:673)
    at View.on (view.js:182)
    at View.fire (emittermixin.js:204)
    at View.change (view.js:475)
    at View._disableRendering (view.js:660)
    at Model.EditingController.listenTo (editingcontroller.js:82)
    at Model.fire (emittermixin.js:204)

GIF

jodator · 2019-02-26T11:05:28Z

Normal

a(146) BLOCKQUOTE                            .OL                            .FIGURE.BLOCKQUOTE
e(153) BLOCKQUOTE.H2    .P     .P     .UL    .OL   .FIGURE.BLOCKQUOTE.OL    .FIGURE.BLOCKQUOTE
d(153) equal     .insert.insert.insert.insert.equal.insert.insert    .insert.equal .equal

first fast diff

actual   (153) BLOCKQUOTE                                                            .OL    .FIGURE.BLOCKQUOTE
expected (160) BLOCKQUOTE.H2    .P     .P     .UL    .OL    .FIGURE.BLOCKQUOTE.OL           .FIGURE.BLOCKQUOTE
diff     (161) equal     .insert.insert.insert.insert.insert.insert.insert    .insert.delete.equal.equal

Second fast diff (different output again)

a(159) BLOCKQUOTE                                                                   .OL    .BLOCKQUOTE
e(167) BLOCKQUOTE.H2    .P     .P     .UL    .OL    .FIGURE.BLOCKQUOTE.OL    .FIGURE       .BLOCKQUOTE
d(168) equal     .insert.insert.insert.insert.insert.insert.insert    .insert.insert.delete.equal

Third fast diff (error)

actual   (166) BLOCKQUOTE.H2    .P     .P     .UL    .OL    .FIGURE.BLOCKQUOTE                         .OL    .FIGURE
expected (174) BLOCKQUOTE.H2    .P     .P     .UL    .OL    .FIGURE.BLOCKQUOTE.OL    .FIGURE.BLOCKQUOTE
diff     (176) equal     .insert.insert.insert.insert.insert.insert.insert    .insert.insert.insert    .delete.delete

On the above the last relevant elements are presented and matched to the diff output. All previous normal diff gives the same output on the last (relevant) places: equal.insert.insert.insert.insert.equal.insert.insert.insert.equal.equal. So the normal diff always matches the <ol> and <figure> and <blockquote> after inserting content in the given list item while the fast diff gives various outputs (probably the second & third one are because of previous wrong diff output.

Reinmar · 2019-02-26T12:18:00Z

Let's extract this to a separate ticket. Doesn't seem to be a critical issue and should not block the release. @Mgsy, could you report a new ticket for it for iteration23?

Fix: Filter out fake selection container before comparing DOM view root children in view renderer. Closes ckeditor/ckeditor5#1578.

jodator added the type:bug This issue reports a buggy (incorrect) behavior. label Feb 25, 2019

Mgsy added the status:confirmed label Feb 25, 2019

Reinmar changed the title ~~Uncough error over multiple paste over an image.~~ Uncaught error over multiple paste over an image. Feb 25, 2019

Reinmar changed the title ~~Uncaught error over multiple paste over an image.~~ Uncaught error over multiple paste over an image Feb 25, 2019

Reinmar added this to the iteration 22 milestone Feb 25, 2019

Reinmar assigned jodator Feb 25, 2019

jodator mentioned this issue Feb 25, 2019

Remove fake selection container before inspecting changed child nodes in Renderer.render(). ckeditor/ckeditor5-engine#1686

Closed

Reinmar changed the title ~~Uncaught error over multiple paste over an image~~ Uncaught error over multiple paste (or other action) over an image Feb 25, 2019

Reinmar mentioned this issue Feb 25, 2019

PoC of how fake selection container can be skipped when diffing nodes ckeditor/ckeditor5-engine#1687

Merged

Mgsy mentioned this issue Feb 26, 2019

Editor crashes after pasting the content multiple times #1579

Closed

jodator closed this as completed in ckeditor/ckeditor5-engine#1687 Feb 26, 2019

jodator added a commit to ckeditor/ckeditor5-engine that referenced this issue Feb 26, 2019

Merge pull request #1687 from ckeditor/t/ckeditor5/1578b

6591f87

Fix: Filter out fake selection container before comparing DOM view root children in view renderer. Closes ckeditor/ckeditor5#1578.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uncaught error over multiple paste (or other action) over an image #1578

Uncaught error over multiple paste (or other action) over an image #1578

jodator commented Feb 25, 2019 •

edited

Loading

Mgsy commented Feb 25, 2019

jodator commented Feb 25, 2019

Reinmar commented Feb 25, 2019

jodator commented Feb 25, 2019

jodator commented Feb 25, 2019 •

edited

Loading

jodator commented Feb 25, 2019

Reinmar commented Feb 25, 2019 •

edited

Loading

Reinmar commented Feb 25, 2019

Reinmar commented Feb 25, 2019

Reinmar commented Feb 25, 2019

Mgsy commented Feb 26, 2019

jodator commented Feb 26, 2019 •

edited

Loading

Reinmar commented Feb 26, 2019

Uncaught error over multiple paste (or other action) over an image #1578

Uncaught error over multiple paste (or other action) over an image #1578

Comments

jodator commented Feb 25, 2019 • edited Loading

Is this a bug report or feature request? (choose one)

💻 Version of CKEditor

📋 Steps to reproduce

✅ Expected result

❎ Actual result

📃 Other details that might be useful

Mgsy commented Feb 25, 2019

jodator commented Feb 25, 2019

Reinmar commented Feb 25, 2019

jodator commented Feb 25, 2019

jodator commented Feb 25, 2019 • edited Loading

jodator commented Feb 25, 2019

Reinmar commented Feb 25, 2019 • edited Loading

Reinmar commented Feb 25, 2019

Reinmar commented Feb 25, 2019

Reinmar commented Feb 25, 2019

Mgsy commented Feb 26, 2019

Steps to reproduce

Error

GIF

jodator commented Feb 26, 2019 • edited Loading

Reinmar commented Feb 26, 2019

jodator commented Feb 25, 2019 •

edited

Loading

jodator commented Feb 25, 2019 •

edited

Loading

Reinmar commented Feb 25, 2019 •

edited

Loading

jodator commented Feb 26, 2019 •

edited

Loading