Closed Bug 998356 Opened 10 years ago Closed 10 years ago

View source is truncated on some parser errors

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla32
Tracking Flags:
Tracking Status
firefox28 --- unaffected
firefox29 - wontfix
firefox30 + verified
firefox31 + verified
firefox32 + verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(1 file)

Don't completely fail out of the parser if we can't format one of our string error messages
2.78 KB, patch
hsivonen
: review+
Sylvestre
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
I came across this while investigating attachment 8408821 [details], but here is a simple testcase:

  view-source:data:text/html,<body><div/></body><div>Can you see me?</div>

This is a regression from bug 943519 and in particular from http://hg.mozilla.org/mozilla-central/rev/bc87301018f7 -- if I back out that changeset locally, view source works correctly.
Flags: needinfo?(hsivonen)
Blocks: 943519
Keywords: regression
So we're ending up in Perform() with eTreeOpAddError.  "msgId" is "errEndWithUnclosedElements", which is not a key present in htmlparser.properties.  So this code:

        rv = nsContentUtils::FormatLocalizedString(
          nsContentUtils::eHTMLPARSER_PROPERTIES, msgId, params, message);
        NS_ENSURE_SUCCESS(rv, rv);

that we hit in the !otherAtom && atom case fails and returns failure from Perform.

Should we be returning NS_OK here if we fail to format the string?  Should there be a string for "errEndWithUnclosedElements"?
My two cents: IMHO there should probably be a string for "errEndWithUnclosedElements"; on the other hand, I'm not quite sure that a missing localisation should mess up a functionality so, yes, I guess it should probably return NS_OK and output to stderr the issue. Probably changing that to NS_ENSURE_SUCCESS(rv, NS_OK)?

But keep in mind that I'm little more than a newbie :)
What does errEndWithUnclosedElements mean?  That is, what should the string say?
Given the code here (http://dxr.mozilla.org/mozilla-central/source/parser/html/javasrc/TreeBuilder.java#6334), it seems to me that errEndWithUnclosedElements is raised whenever an opening HTML tag is found without the matching closing tag before EOF is seen.
Not tracking for 29 (too late) but tracking for 30 & 31.
status-firefox29: affected → wontfix
tracking-firefox29: ? → -
tracking-firefox30: ? → +
tracking-firefox31: ? → +
(In reply to Boris Zbarsky [:bz] from comment #1)
> Should we be returning NS_OK here if we fail to format the string? 

Depends on what failure mode we want for missing strings. I guess returning NS_OK would be more graceful.

> Should
> there be a string for "errEndWithUnclosedElements"?

Apparently, yes!

(In reply to Boris Zbarsky [:bz] from comment #3)
> What does errEndWithUnclosedElements mean?  That is, what should the string
> say?

End tag for “%1$S” seen, but there were unclosed elements.
Flags: needinfo?(hsivonen)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Attachment #8425961 - Flags: review?(hsivonen) → review+
Triage drive-by: this is tracked for FF30 and so would need to be ready (nominated) for uplift before next Tuesday morning Pacific time in order to make Beta.
status-firefox32: --- → affected
tracking-firefox32: --- → +
Comment on attachment 8425961 [details] [diff] [review]
Don't completely fail out of the parser if we can't format one of our string error messages

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 943519
User impact if declined: view-source will sometimes show only part of the source
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): Low risk, imo.
String or IDL/UUID changes made by this patch: There is a string addition, but we
   could leave that out of the patch that lands on beta.  The other part of the
   patch is enough to fix the bug on its own.
Attachment #8425961 - Flags: approval-mozilla-beta?
Attachment #8425961 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b45a3f742d8
Flags: in-testsuite?
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/0b45a3f742d8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox32: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8425961 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8425961 [details] [diff] [review]
Don't completely fail out of the parser if we can't format one of our string error messages

a=me for a patch without the string change for Beta.
Attachment #8425961 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I reproduced the issue with Nightly from April 2nd (BuildID: 20140402030201).
The issue no longer reproduces with: FF 30 Beta 8 (BuildID: 20140527133511), latest FF 31.0a2 Aurora (BuildID: 20140528004008), and latest FF 32.0a1 Nightly (BuildID: 20140527030202).

Verified on Win 7 x64, Mac OS X 10.9, Ubuntu 13.10 x86.
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
status-firefox32: fixed → verified
It's the second time I see a patch without strings landing on beta, and then a patch with a string change landing on aurora. Aurora is supposed to be string frozen exactly like beta, what am I missing?
Ryan - can you back out the patch with string changes from Aurora and re-land the one without string changes?
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: