Bug 85703 – Web Inspector does not display long single line documents correctly

• Component: Layout and Rendering
• Status: REOPENED

[Andrey Adaikin]

Created attachment 140389 [details]
test case

Inline text boxes do not properly display texts longer that 65535 chars.

Simple test case below:

<pre id="pre"></pre>
<script>
  var text = 'a';
  for (var i = 0; i < 16; ++i) {
    text += text;
  }
  text += text.length; // Length is more than 65536
  document.getElementById("pre").textContent = text;
</script>

[Andrey Adaikin]

Created attachment 140390 [details]
Patch

[Andrey Adaikin]

Created attachment 140391 [details]
Patch

[Pavel Feldman]

Comment on attachment 140391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140391&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:133
> +        bool start = (state != RenderObject::SelectionEnd && startPos >= m_start && startPos - m_start < (int)m_len);

Use static_cast instead.

> Source/WebCore/rendering/InlineTextBox.cpp:194
> +    bool respectHyphen = ePos == (int)m_len && hasHyphen();

Why did this change?

> Source/WebCore/rendering/InlineTextBox.cpp:1268
> +    ASSERT(offset - m_start <= (int)m_len);

static_cast

> Source/WebCore/rendering/InlineTextBox.h:169
> +    unsigned m_len;

Every bit in this struct counts, you can't simply bump the sizes here.

[Andrey Adaikin]

Comment on attachment 140391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140391&action=review

>> Source/WebCore/rendering/InlineTextBox.cpp:133
>> +        bool start = (state != RenderObject::SelectionEnd && startPos >= m_start && startPos - m_start < (int)m_len);
> 
> Use static_cast instead.

I was just being consistent with the code around - there are plenty of "(int)m_len" in this code, and no static_cast.

>> Source/WebCore/rendering/InlineTextBox.cpp:194
>> +    bool respectHyphen = ePos == (int)m_len && hasHyphen();
> 
> Why did this change?

Otherwise compile-time error: comparison between signed and unsigned integer expressions

>> Source/WebCore/rendering/InlineTextBox.h:169
>> +    unsigned m_len;
> 
> Every bit in this struct counts, you can't simply bump the sizes here.

Hm. What do you suggest then? :) I doubt 65K is enough a limit for a text size of an element.

[Andrey Adaikin]

Anyone?

[Antti Koivisto]

(In reply to comment #4)
> Hm. What do you suggest then? :) I doubt 65K is enough a limit for a text size of an element.

There is no such limit for elements. Your patch is about maximum length of a single line. 

WONTFIX due to lack of explanation why this is a problem we would like to solve.

[Andrey Adaikin]

(In reply to comment #6)
> WONTFIX due to lack of explanation why this is a problem we would like to solve.

So I thought this was merely a bug that a page like this https://bug-85703-attachments.webkit.org/attachment.cgi?id=140389 would not render the text content in full length (unlike some other web browsers). If it does work as intended, I don't have any other explanation of why this should be a problem :)

[Eric Seidel (no email)]

I'm not sure what's happening in that test case.  I wonder if we're overflowing an unsigned short.  It appears to be what's suggested by the test.

[Emil A Eklund]

(In reply to comment #8)
> I'm not sure what's happening in that test case.  I wonder if we're overflowing an unsigned short.  It appears to be what's suggested by the test.

That seems to be the case 
InlineTextBox::m_len is an unsigned short, as is the m_truncate field that controls the text truncation point.

Making both of those ints would greatly reduce the memory usage for inline text boxes with is probably undesirable. I suppose a different approach would be to detect the overflow and break it into two/several InlineTextBox objects. This would be exposed through the DOM but seems more reasonable than the current failure case.

[Andrey Adaikin]

(In reply to comment #8)
> I'm not sure what's happening in that test case.  I wonder if we're overflowing an unsigned short.  It appears to be what's suggested by the test.

Yes, exactly. The setLen() setter accepts an unsigned int, while m_len is declared unsigned short. In this test case the length of the text is (65536+5) as seen in the setter, which will be just 5 in m_len, thus we display only 5 chars in the test. So, isn't it a bug?

[Levi Weintraub]

(In reply to comment #9)
> Making both of those ints would greatly reduce the memory usage for inline text boxes with is probably undesirable.

s/reduce/increase/

Out of curiosity, did you come across this while looking at/building a page? While I've seen this code, I've never seen a case where it wrapped in the wild.

[Andrey Adaikin]

(In reply to comment #11)
> (In reply to comment #9)
> > Making both of those ints would greatly reduce the memory usage for inline text boxes with is probably undesirable.
> 
> s/reduce/increase/

BTW, if the memory usage is so critical here, why the m_start declared as int, but not unsigned short? I assume it should always be the case that (0 <= m_start < m_len), so if m_len is unsigned short, why m_start should be a signed int?

> Out of curiosity, did you come across this while looking at/building a page? While I've seen this code, I've never seen a case where it wrapped in the wild.

From this issue: http://code.google.com/p/chromium/issues/detail?id=84980
Of course, I can fix this particular issue locally in the Web Inspector code, but obviously the problem is wider than that.

[Antti Koivisto]

Reopening and retitling as the Web Inspector case seems like a real world issue. 

It would probably be best to fix this in Inspector. 64k text lines are not very usable. If there is a need to lift the WebKit 64k character line length limit then the right way to do it is indeed to create multiple inline boxes per line, not to bloat the common case.

[Antti Koivisto]

(In reply to comment #12)
> BTW, if the memory usage is so critical here, why the m_start declared as int, but not unsigned short? I assume it should always be the case that (0 <= m_start < m_len), so if m_len is unsigned short, why m_start should be a signed int?

m_start is the index from the start of the block, not the line.

[Andrey Adaikin]

(In reply to comment #13)
> Reopening and retitling as the Web Inspector case seems like a real world issue. 
> 
> It would probably be best to fix this in Inspector. 64k text lines are not very usable. If there is a need to lift the WebKit 64k character line length limit then the right way to do it is indeed to create multiple inline boxes per line, not to bloat the common case.

Yeah, agreed. I'd rather chose the latter, i.e. creating multiple boxes per line in the InlineTextBox code instead of just fixing the particular Web Inspector case. If there is no objections on this, I'll investigate into the code to find out how to do this right.

[Aaron Boushley]

I have found at least one instance of this in the wild (outside of the web inspector).

Would a patch that breaks these long lines into multiple inline text boxes be acceptable?

[Andrey Adaikin]

(In reply to comment #16)
> I have found at least one instance of this in the wild (outside of the web inspector).

Could you please provide a link or repro steps?

[Aaron Boushley]

The repro steps are essentially the same as the content in the already attached test case.

In our scenario users upload JSON files to interact with our service. There is an area where they can view those files. When users upload large JSON files that have been minified we run into this problem. The problem is made worse by the fact that the container is aware of the full width and thus lets you scroll the entire distance, but the text is truncated.

[Levi Weintraub]

(In reply to comment #15)
> (In reply to comment #13)
> > Reopening and retitling as the Web Inspector case seems like a real world issue. 
> > 
> > It would probably be best to fix this in Inspector. 64k text lines are not very usable. If there is a need to lift the WebKit 64k character line length limit then the right way to do it is indeed to create multiple inline boxes per line, not to bloat the common case.
> 
> Yeah, agreed. I'd rather chose the latter, i.e. creating multiple boxes per line in the InlineTextBox code instead of just fixing the particular Web Inspector case. If there is no objections on this, I'll investigate into the code to find out how to do this right.

I think this is the best solution, and an improvement over the current behavior.