Last Comment Bug 43267 - view source displays attributes with quotes in strange places incorrectly
: view source displays attributes with quotes in strange places incorrectly
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: P3 minor (vote)
: Future
Assigned To: Blake Kaplan (:mrbkap)
: Moied
:
Mentors:
: 92196 179569 182215 184816 213299 220386 262562 265638 270792 271217 276972 292253 (view as bug list)
Depends on: 57724
Blocks:
  Show dependency treegraph
 
Reported: 2000-06-20 22:23 PDT by Jeremy M. Dolan
Modified: 2005-04-28 13:31 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (123 bytes, text/html)
2000-06-20 22:43 PDT, Jeremy M. Dolan
no flags Details
patch v1 (4.38 KB, patch)
2004-09-18 11:53 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
rbs: superreview+
Details | Diff | Splinter Review
more testcases (779 bytes, text/html)
2004-09-23 11:05 PDT, Blake Kaplan (:mrbkap)
no flags Details
[screenshot] FYI - parsing of the testcase - before & after (21.48 KB, image/png)
2004-09-28 01:07 PDT, rbs
no flags Details

Description Jeremy M. Dolan 2000-06-20 22:23:18 PDT
In some cases, <frameset ...> being shown wrong. Test case will follow.
Comment 1 Jeremy M. Dolan 2000-06-20 22:42:02 PDT
OK, looks like this is caused by invalid HTML. Misplaced, unmatched quotes. It's
amazing it actually renders. Regardless, view source should have no problems
with it. 4.7 view source is fine.

The " at the end of frameborder is being cut, and the space after that.

4.7 is displaying the initial " colored as the same color as text, which is
probably the best we can hope to do.
Comment 2 Jeremy M. Dolan 2000-06-20 22:43:09 PDT
Created attachment 10443 [details]
testcase
Comment 3 Doron Rosenberg (IBM) 2000-06-21 09:36:16 PDT
--> xp apps.

sairuh - any ideas where this really belongs? Perhaps networking? According to
the component page, its xp apps gui, but I seem to doubt it is ben's fault.
Comment 4 sairuh (rarely reading bugmail) 2000-06-21 15:30:02 PDT
here's the source (of the testcase) i see using View Page Source in 4.73 on
linux:

<html>
<frameset "frameborder="0" extra attributes here>
<frame src="foo.html">
<frame src="bar.html">
</frameset>
</html>

here's what i see on using today's linux commercial bits 2000.06.21.08:

<html>
<frameset "frameborder="0extra attributes here>
<frame src="foo.html">
<frame src="bar.html">
</frameset>
</html>

methinks this should go over to the Parser group...
Comment 5 harishd 2000-06-21 15:39:27 PDT
Let me investigate if it's parser. Over to me.

Btw, thanx for the test case.
Comment 6 harishd 2000-06-21 19:24:05 PDT
This is a very low priority bug. Moving to FUTURE list.
Comment 7 rickg 2000-06-22 22:32:07 PDT
Harish -- First, I agree with your FUTURE approach to this. Second, if we 
wanted to fix it, we could use code I added a while back to capture ws between 
attributes so that viewsource would render correctly. I suspect we can use the 
same mechanism to capture the (invalid) second ". 
Comment 8 Jesse Ruderman 2000-12-16 16:45:16 PST
Adding to bug 57724, tracker for view-source displaying text incorrectly.


On Win98 2000121608, I see
<frameset frameborder="0" extra attributes here>

Reported earlier (for Linux):
<frameset "frameborder="0extra attributes here>

Correct:
<frameset "frameborder="0" extra attributes here>

Has it changed, or is it different on different platforms?
Comment 9 Ben Bucksch (:BenB) 2001-01-01 05:28:37 PST
Testcase gives me

Not Found
The requested URL /foo.html was not found on this server.

Apache/1.3.12 Server at bugzilla.mozilla.org Port 80
Comment 10 bsharma 2001-03-02 15:41:40 PST
updated qa contact.
Comment 11 Manoj 2001-06-22 09:25:19 PDT
When viewing the page source, the source does not wrap to the size of the
window. Could someone look into making the lines wrap to the window. This makes
reading the source easier than it is right now. Also, is it possible to specify
which editor should be used to read the source (though this may not be platform
independent)
Comment 12 Jeremy M. Dolan 2001-06-22 09:48:37 PDT
+dataloss, though it's a borderline case.

Jesse: Today's linux is broken in the way win98 was for you, 6 months ago.

Ben: ignore that, the issue is with view source, yes the frame targets are 404s.

Manoj: this isn't the place for that request. That should be a new bug with
serverity=enhancement, if there isn't one already (pretty sure there is).

All: should this depend on 55583 or the other one that's regarding retrieving
original non-parsed source for things like 'save'?
Comment 13 Jesse Ruderman 2001-06-22 12:27:09 PDT
jmd: Fixing bug 55583 wouldn't fix all of the view source munging problems,
because view source would still go through the parser to become colored.
Comment 14 Jeremy M. Dolan 2001-06-22 12:33:47 PDT
non-colored view source is still being run through the parser? this bug still
shows with syntax coloring off...
Comment 15 Andreas M. "Clarence" Schneider 2001-07-20 13:12:38 PDT

*** This bug has been marked as a duplicate of 57724 ***
Comment 16 Christopher Hoess (gone) 2002-01-13 13:41:54 PST
Reopening 57724 dependencies for independent resolution.
Comment 17 Christopher Hoess (gone) 2002-01-13 13:58:54 PST
Works fine on today's Linux CVS with and without syntax highlighting.  Resolving
WORKSFORME.
Comment 18 Jeremy M. Dolan 2002-01-13 14:12:03 PST
I still see this. My testcase line should read:

<frameset "frameborder="0" extra attributes here>
          ^
view source isn't showing the first quote.

Reopening on choess' request.
Comment 19 Soren Ragsdale 2002-10-06 13:44:12 PDT
I believe I'm experiencing this same bug.  My HTML code contains:

================================================
<form action="foo.cgi">
<input type="hidden" name="tagline" value="begin \"double\" \'single\' <inside> 
\\slash\\ end">;
<input type="submit" value="Save">
</form>
================================================

...but the "view source" display shows:

++++++++++++++++++++++++++++++++++++++++++++++++
<form action="openlist.phtml">
<input type="hidden" name="tagline" value="\"double\ \'single\' ><inside>
\\slash\\ xxx">;
<input type="submit" value="Save">
</form>
+++++++++++++++++++++++++++++++++++++++++++++++

The second quote mark has been removed.
Also please note the extra > that's been added.

Disclaimer: I'm not completely sure that the above is valid HTML code.  I think
this is the way to escape single quotes, double quotes, backslashes, and
brackets but it's possible that this isn't valid and the bug is on *my* end.
Comment 20 André Dahlqvist 2002-11-11 15:06:16 PST
*** Bug 179569 has been marked as a duplicate of this bug. ***
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2002-12-05 15:13:55 PST
The correct way to escape a quote is &quot;, not \".  But this is still a problem.
Comment 22 Christopher Hoess (gone) 2002-12-11 15:07:46 PST
*** Bug 184816 has been marked as a duplicate of this bug. ***
Comment 23 Brant Gurganus 2003-01-18 17:47:54 PST
By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.
Comment 24 Christopher Hoess (gone) 2003-01-18 18:58:23 PST
Darned if I know how "dataloss" got stuck on this. Re-adjusting.
Comment 25 Jeremy M. Dolan 2003-01-18 19:11:17 PST
It's dataloss because users and developers should rightfully assume pasting text
from view source is the same as saving that portion of the file.

If a "view" function is *changing* data, data may be lost.
Comment 26 Christopher Hoess (gone) 2003-01-18 19:25:10 PST
We have about a gajilion of these little bugs (mostly deps of bug 57724; I just
created bug 188609 for the root cause of this mess). With all due respect, I
don't think they meet the criterion of *critical* data loss, which (AFAIK) is
meant for stuff like wiping out your form data and doubling your credit card
order, etc. I agree it's a problem, but realistically only fixing bug 188609 is
going to solve this stuff.
Comment 27 Blake Kaplan (:mrbkap) 2004-09-15 16:26:56 PDT
*** Bug 92196 has been marked as a duplicate of this bug. ***
Comment 28 Blake Kaplan (:mrbkap) 2004-09-16 00:00:18 PDT
*** Bug 182215 has been marked as a duplicate of this bug. ***
Comment 29 Blake Kaplan (:mrbkap) 2004-09-18 11:53:14 PDT
Created attachment 159332 [details] [diff] [review]
patch v1

Some really fun stuff here...

This makes us not lose misplaced quotes in attributes. There are issues with
newline counting after invalid attributes, but that's another bug altogether.
I've tried to keep the behavior of the tokenizer the same in non-view-source
mode. The only real change is that in ConsumeQuotedString, I no longer call
SkipOver(). In non-view-source mode, subsequent quotes will be handled by the
SkipOver call in CAttributeToken::Consume() (when we try to consume the next
attribute).
Comment 30 Blake Kaplan (:mrbkap) 2004-09-18 11:56:46 PDT
Comment on attachment 159332 [details] [diff] [review]
patch v1

bz, could you please give this r=? I'm holding off on asking for sr= until I
have r=.
Comment 31 Blake Kaplan (:mrbkap) 2004-09-21 12:46:23 PDT
*** Bug 220386 has been marked as a duplicate of this bug. ***
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2004-09-23 08:56:39 PDT
Comment on attachment 159332 [details] [diff] [review]
patch v1

Yeah, this looks reasonable.  I'd like to see some testcases attached to this
bug (and maybe checked in as regression tests if we have such for parser?)
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2004-09-23 08:57:28 PDT
Comment on attachment 159332 [details] [diff] [review]
patch v1

rbs, would you sr?
Comment 34 Blake Kaplan (:mrbkap) 2004-09-23 11:05:50 PDT
Created attachment 159873 [details]
more testcases

This should cover most of the cases. One thing this uncovers is that though we
don't allow " to be in attribute names, we do allow ' to be. Is this desired?
There does seem to be a parser regression test suite, but I can't get it to
run.
Comment 35 rbs 2004-09-28 01:07:25 PDT
Created attachment 160312 [details]
[screenshot] FYI - parsing of the testcase - before & after

There are subtle differences, goto line 9:45 on the Source-of, or the 'a='b''
becomes 'a="b" '="" in the DOM-Source. But I don't mind either way unless other
people have objections.

Regarding the patch, why do you GetChar() instead of SkipOver(). Looks like you
copy the same char (quote) over itself, no? What specific problem does it fix?
Comment 36 Blake Kaplan (:mrbkap) 2004-09-28 08:26:38 PDT
(In reply to comment #35)
> There are subtle differences, goto line 9:45 on the Source-of, or the 'a='b''
> becomes 'a="b" '="" in the DOM-Source. But I don't mind either way unless other
> people have objections.

Once I fix bug 261503 this is going to turn into the same thing as when we have
extra double-quotes (technically, it should turn into ="" in the DOM source and
when we parse HTML for the real content model as opposed to view-source, those
extraneous quotes should be removed).

Once that's fixed, my eventual plan is to nuke the attributes that generate the
source |=""| in CNavDTD (after getting their newline count) since this makes us
assert (since these attributes have no name).

> 
> Regarding the patch, why do you GetChar() instead of SkipOver(). Looks like you
> copy the same char (quote) over itself, no? What specific problem does it fix?

If you're referring to ConsumeQuotedString, the reason is that if we have: <a
href="b""> then that call to SkipOver loses the second quote, so view-source
misses it. I need to actually GetChar() on it because ConsumeUntil doesn't
actually consume the end condition.

bz, do you have any problems with the new parsing?
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2004-09-28 08:47:47 PDT
No, I think the new parsing of such totally bogus stuff is OK (and will get
better once bug 261503 is fixed).
Comment 38 Paul Chamberlain 2004-09-28 14:48:13 PDT
For additional context...

My symptom for this problem was in WYSIWYG Pro (a JS-based HTML editor).
I was trying to coerce it into service as a template editor, but when I
put certain Smarty tags inside an attribute the editor was destructive
to the content that I was editing, causing Smarty to fail.

I eventually determined that it was Mozilla (gecko?) whose parser was
getting confused.  It's my theory that if the parser did a better job
of ignoring what it didn't understand, I might succeed in my original
goal.
Comment 39 rbs 2004-09-28 20:45:41 PDT
Comment on attachment 159332 [details] [diff] [review]
patch v1

sr=rbs

I now see why you needed to turn SkipOver into GetChar. The skip _loops_ until
it encounter a different char...

Patch looks fine. Not much to expect from sloppy markups.
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-09-29 20:02:35 PDT
checked in
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-09-29 20:02:44 PDT
checked in
Comment 42 Blake Kaplan (:mrbkap) 2004-10-04 14:30:51 PDT
*** Bug 262562 has been marked as a duplicate of this bug. ***
Comment 43 :Gavin Sharp [email: gavin@gavinsharp.com] 2004-10-04 14:50:30 PDT
Can this fix be landed on aviary?
Comment 44 Boris Zbarsky [:bz] (still a bit busy) 2004-10-04 16:06:12 PDT
No.  This is very much an alpha change.
Comment 45 Blake Kaplan (:mrbkap) 2004-10-05 23:29:22 PDT
*** Bug 213299 has been marked as a duplicate of this bug. ***
Comment 46 Blake Kaplan (:mrbkap) 2004-10-23 08:23:07 PDT
*** Bug 265638 has been marked as a duplicate of this bug. ***
Comment 47 Gary van der Merwe 2004-11-19 06:06:22 PST
*** Bug 270792 has been marked as a duplicate of this bug. ***
Comment 48 Blake Kaplan (:mrbkap) 2004-11-22 06:34:37 PST
*** Bug 271217 has been marked as a duplicate of this bug. ***
Comment 49 Daniel Faber 2004-12-02 02:06:32 PST
Looking at the attachment "more testcases" with Firefox 1.0 (Gentoo Linux), I
have the same problems again.  None of the badly quoted attributes are displayed
correctly in the view source window.  That's very bad for webdesingers debugging
there dynamicly generated HTML code.  Can anybody confirm these problems with
the latest Firefox release?
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2004-12-02 07:22:46 PST
Daniel, Firefox 1.0 is using the Gecko engine from last April.  None of these
fixes are in Firefox 1.0.  They will be in Firefox 1.1.
Comment 51 Phil Ringnalda (:philor) 2005-01-04 09:18:07 PST
*** Bug 276972 has been marked as a duplicate of this bug. ***
Comment 52 Tracy Walker [:tracy] 2005-01-11 10:40:33 PST
verified with mozilla 1.8a6 2005-01-11-05-trunk
Comment 53 Blake Kaplan (:mrbkap) 2005-03-12 22:25:16 PST
*** Bug 285888 has been marked as a duplicate of this bug. ***
Comment 54 Blake Kaplan (:mrbkap) 2005-03-14 10:44:35 PST
*** Bug 286109 has been marked as a duplicate of this bug. ***
Comment 55 Phil Ringnalda (:philor) 2005-04-28 13:31:24 PDT
*** Bug 292253 has been marked as a duplicate of this bug. ***

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