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

RESOLVED FIXED in mozilla15

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: O. Atsushi (Torisugari))

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla15
assertion, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 615136 [details]
testcase

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

Comment 1

5 years ago
Created attachment 615137 [details]
stack trace
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
Created attachment 620674 [details] [diff] [review]
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?
Attachment #620674 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 4

5 years ago
Created attachment 620675 [details]
simple fix v2

Sorry that was wrong file.
Attachment #620674 - Attachment is obsolete: true
Attachment #620674 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 5

5 years ago
Created attachment 620677 [details] [diff] [review]
simple fix v3
Attachment #620675 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620677 - Flags: review?(justin.lebar+bug)
> 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.
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 on attachment 620677 [details] [diff] [review]
simple fix v3

And by "bz" I meant "a docshell peer".  :)
Attachment #620677 - Flags: review?(justin.lebar+bug)
Attachment #620677 - Flags: review?(bugs)
Attachment #620677 - Flags: feedback+

Comment 9

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

r=me, but make the changes Justin suggested.
Attachment #620677 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 620758 [details] [diff] [review]
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.
Attachment #620677 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620758 - Flags: review?(bugs)

Updated

5 years ago
Attachment #620758 - Flags: review?(bugs) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86_64 → All
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a5cc532b2b

Should this have a crashtest?
Assignee: nobody → torisugari
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/48a5cc532b2b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.