Last Comment Bug 745530 - "ASSERTION: can't scroll to empty anchor name" with null char
: "ASSERTION: can't scroll to empty anchor name" with null char
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: O. Atsushi (Torisugari)
:
Mentors:
Depends on:
Blocks: 343943
  Show dependency treegraph
 
Reported: 2012-04-15 04:41 PDT by Jesse Ruderman
Modified: 2012-05-09 03:44 PDT (History)
4 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (142 bytes, text/html)
2012-04-15 04:41 PDT, Jesse Ruderman
no flags Details
stack trace (11.99 KB, text/plain)
2012-04-15 04:41 PDT, Jesse Ruderman
no flags Details
simple fix (1.20 KB, patch)
2012-05-03 06:17 PDT, O. Atsushi (Torisugari)
no flags Details | Diff | Review
simple fix v2 (1.20 KB, text/plain)
2012-05-03 06:19 PDT, O. Atsushi (Torisugari)
no flags Details
simple fix v3 (1.20 KB, patch)
2012-05-03 06:25 PDT, O. Atsushi (Torisugari)
bugs: review+
justin.lebar+bug: feedback+
Details | Diff | Review
simple fix v4 (1.31 KB, patch)
2012-05-03 10:22 PDT, O. Atsushi (Torisugari)
bugs: review+
Details | Diff | Review

Description Jesse Ruderman 2012-04-15 04:41:03 PDT
Created attachment 615136 [details]
testcase

###!!! ASSERTION: can't scroll to empty anchor name: '!aScroll', file layout/base/nsPresShell.cpp, line 2953
Comment 1 Jesse Ruderman 2012-04-15 04:41:19 PDT
Created attachment 615137 [details]
stack trace
Comment 2 O. Atsushi (Torisugari) 2012-05-02 19:55:54 PDT
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) 2012-05-03 06:17:07 PDT
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?
Comment 4 O. Atsushi (Torisugari) 2012-05-03 06:19:04 PDT
Created attachment 620675 [details]
simple fix v2

Sorry that was wrong file.
Comment 5 O. Atsushi (Torisugari) 2012-05-03 06:25:57 PDT
Created attachment 620677 [details] [diff] [review]
simple fix v3
Comment 6 Justin Lebar (not reading bugmail) 2012-05-03 08:39:01 PDT
> 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) 2012-05-03 08:48:03 PDT
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) 2012-05-03 08:48:36 PDT
Comment on attachment 620677 [details] [diff] [review]
simple fix v3

And by "bz" I meant "a docshell peer".  :)
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-03 09:14:49 PDT
Comment on attachment 620677 [details] [diff] [review]
simple fix v3

r=me, but make the changes Justin suggested.
Comment 10 O. Atsushi (Torisugari) 2012-05-03 10:22:40 PDT
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.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-05-08 15:51:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a5cc532b2b

Should this have a crashtest?
Comment 12 Ed Morley [:emorley] 2012-05-09 03:44:23 PDT
https://hg.mozilla.org/mozilla-central/rev/48a5cc532b2b

Note You need to log in before you can comment on or make changes to this bug.