745530 - "ASSERTION: can't scroll to empty anchor name" with null char

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: can't scroll to empty anchor name: '!aScroll', file layout/base/nsPresShell.cpp, line 2953

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[O. Atsushi (Torisugari)]

The raw string is "#\x00"
nsStandardURL (probably) converts it to "#%00"

Before calling nsUnescape(...), ref does exist.     '#' + "%00"
After calling nsUnescape(...), ref is empty string. '#' + '/0'

And utf-8 mode is checking whether it's empty or not. The problem is charset-conversion mode. So all we should do to fix this bug is add one more empty string check.

The remaining issues are...

1.
Charset conversion is really required? IIRC, regardless of document charset, URIs don't copy raw strings any longer. It should always escaped UTF-8.

2.
Bug 308590 is fixed. It's time to switch to nsIURI::GetRef()? (bug 126432)

Comment 3

[O. Atsushi (Torisugari)]

Attachment: simple fix

By the way, I don't understand why this bug can block other bugs, for PresShell::GoToAnchor() catches the error successfully. Doesn't it?

Comment 4

[O. Atsushi (Torisugari)]

Attachment: simple fix v2

Sorry that was wrong file.

Comment 5

[O. Atsushi (Torisugari)]

Attachment: simple fix v3

Comment 6

[Justin Lebar (not reading bugmail)]

> By the way, I don't understand why this bug can block other bugs,

Yeah, I was confused too.  I'm don't think I'm not spoiling any secrets by saying that bug 343943 is a metabug for a fuzzer.

Comment 7

[Justin Lebar (not reading bugmail)]

I'd like bz to look at this, just to be safe.

> +            // When newHashName contains "%00", unescaped string may be empty.

Please elaborate, along the lines of "and GoToAnchor asserts if we ask it to scroll to an empty ref."

> +            scroll &= (!uStr.IsEmpty());

So there's no question about datatypes and bitwise &, would you mind changing this to |scroll = scroll && !uStr.IsEmpty()|?

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 620677 [details] [diff] [review]
simple fix v3

And by "bz" I meant "a docshell peer".  :)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 620677 [details] [diff] [review]
simple fix v3

r=me, but make the changes Justin suggested.

Comment 10

[O. Atsushi (Torisugari)]

Attachment: simple fix v4

Addressed comment #7 an comment #9

> bug 343943 is a metabug for a fuzzer.

Aha, that makes sense. That is, the goal or this bug is merely shut up the assertion and nothing else.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48a5cc532b2b

Should this have a crashtest?

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/48a5cc532b2b