Closed Bug 515109 Opened 15 years ago Closed 3 months ago

DTD parser breaks if chrome/jar file not found (breaks about:rights in Fennec trunk)

Categories

(Core :: XML, defect)

Product:
Core
Component:
XML
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: Gavin, Unassigned)

References

Details

(Keywords: regression)

Attachments

(1 obsolete file) 

Seems like brand.dtd is failing to load somehow. This doesn't occur on 1.9.2.
So I debugged this and found that after security.dtd fails to load, the last dtd (ie aboutRights.dtd) no longer gets loaded at all (from nsExpatDriver.cpp). I'm not sure why that doesn't fail just as hard in 1.9.2. I did verify that swapping the two dtd's in the xhtml file fixes the issue. I'll have a look if I can figure out why this is broken when it didn't used to be... Might be that the expat bits would need to be fixed rather than swapping the dtds as a workaround.
OK, I think I found the culprit. Expat attempts to load the security.dtd file. On trunk, that leads to the following stack of calls:

nsExpatDriver::HandleExternalEntityRef (nsExpatDriver.cpp:734)
nsExpatDriver::OpenInputStreamFromExternalDTD (nsExpatDriver.cpp:825)
NS_OpenURI (/nsNetUtil.h:228)
nsJARChannel::Open (nsJARChannel.cpp:677)

because of bug 512515, on trunk this now returns NS_FILE_NOT_FOUND, whereas on branch nothing happens (it just returns NS_OK at the end of the method).

I *think* the proper fix is to get expat to not balk completely about NS_FILE_NOT_FOUND, and continue loading the rest of the DTD references, but I'd like a second opinion. :-)

For now, marking this as depending on bug 512515.
Depends on: 512515
Keywords: regression
Blocks: 512515
No longer depends on: 512515
(In reply to comment #3)
> I *think* the proper fix is to get expat to not balk completely about
> NS_FILE_NOT_FOUND, and continue loading the rest of the DTD references, but I'd
> like a second opinion. :-)

Sounds right to me.
Attached patch Rough patch (obsolete) — — Splinter Review 
So, it's actually slightly more subtle still, as whether parsing continues depends on some internal expat stuff. Just returning "1" (as the NS_ENSURE_SUCCESS calls actually do) isn't enough and leads to parsing being stopped. IIRC, on branch, we still create a stream, it's just empty and nothing gets parsed. This patch mimics that behaviour on trunk, although I'm not sure if this approach is entirely correct. I'm happy to take suggestions on how to make this better if necessary - requesting review to get such suggestions :-). owners.html suggests peterv, if I should be asking someone else, please let me know...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #414245 - Flags: review?(peterv)
Assignee: gijskruitbosch+bugs → nobody
Component: General → XML
Product: Fennec → Core
QA Contact: general → xml
Summary: about:rights broken in Fennec trunk builds → DTD parser breaks if chrome/jar file not found (breaks about:rights in Fennec trunk)
Peter, could you have a look at this? If you're busy, can you suggest someone else to take the review? Thanks!
Can we get a NS_WARNING if the dtd isn't found? Seems like we shouldn't normally try to point to missing dtds if we can help it.
This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Status: ASSIGNED → NEW
Peter: any chance of a review on this? :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Reproducible with the build identifier: 

Mozilla /5.0 (Maemo;Linux armv7l;rv:2.0b6pre)Gecko/20100913 Firefox/4.0b6pre Fennec /2.0b1pre
@Peter: any chance to get a review on this patch? It's been over a year. :-)

@Everyone else: suggestions for alternative reviewers/courses of action here?
Comment on attachment 414245 [details] [diff] [review]
Rough patch

Why do we need to create the external entity parser if we don't have a stream? That seems wrong.
Attachment #414245 - Attachment is obsolete: true
Attachment #414245 - Flags: review?(peterv)
Assignee: gijskruitbosch+bugs → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW

Axel, should we go ahead and parse the rest of the DTDs even if one of them fails to load? We do parse the rest of the document... Simple testcase:

<?xml version="1.0"?>
<!DOCTYPE html [
<!ENTITY % fakeDTD SYSTEM "foo:bar"> %fakeDTD;
<!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd"> %globalDTD;
]>
<root>&locale.dir;</root>

This came up in https://phabricator.services.mozilla.com/D34656 though it's not the main thing going wrong there.

Flags: needinfo?(l10n)

I think that'd be a good idea. We should probably report to the console still, because, by all likelyhood, the parser is gonna break later on missing entities ;-).

Flags: needinfo?(l10n)
Severity: normal → S3

No more DTDs, whee!

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: