1227554 - DOM Inspector is non functional due to RDF Errors

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Philip Chee]

Error: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIRDFContainerUtils.IndexToOrdinalResource]
Source file: chrome://inspector/content/jsutil/rdf/RDFU.js
Line: 28

Error: undefined entity
Source file: chrome://inspector/content/res/viewer-registry.rdf
Line: 43, Column: 13
Source code:
<rdf:li><rdf:Description

Comment 1

[Philip Chee]

Alice0775 Can you find the regression range? Thanks.

Comment 2

[Alice0775 White]

(In reply to Philip Chee from comment #1)
> Alice0775 Can you find the regression range? Thanks.

I cannot reproduce the error.

Comment 3

[Alice0775 White]

OK, I can repro without e10s. but not with e10s. 

STR
1. Disabled e10s
2. Install DOM Inspector 2.0.16 and restart
3. Open DOM Inspector from menu

Comment 4

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0eab88b18c27ac2fa767725f3b6268c0aeb320e&tochange=9c2bad72beab

Regressed by: Bug 1182546

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Broken extension that probably shouldn't have been broken by the changes...

Unless this extension is linking to this DTD from untrusted content of some sort, of course.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Philip, any chance you could post the console error you are seing?

Comment 7

[Philip Chee]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Philip, any chance you could post the console error you are seeing?

The only errors are in Comment 0

(In reply to Philip Chee from comment #0)
> Error: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIRDFContainerUtils.IndexToOrdinalResource]
> Source file: chrome://inspector/content/jsutil/rdf/RDFU.js
> Line: 28
> 
> Error: undefined entity
> Source file: chrome://inspector/content/res/viewer-registry.rdf
> Line: 43, Column: 13
> Source code:
> <rdf:li><rdf:Description

The source code is @ http://hg.mozilla.org/dom-inspector/

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/resources/content/res/viewer-registry.rdf?rev=d96d6863fe08&mark=43-43#42

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/resources/content/jsutil/rdf/RDFU.js?rev=6c425834882f&mark=28-28#26

The DTD bz mentions is probably:
http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/resources/content/res/viewer-registry.rdf?rev=d96d6863fe08&mark=7-7#7

Comment 8

[Philip Chee]

Even with Bug 1228116 fix DOM Inspector still doesn't work

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1227554_expatdriver_default_nullprincipal.patch

Boris, in the initial review you wrote:
> In the other cases, we should be using the principal of the document,
> or the null principal if there is no document (assuming that can happen at all).

Now we know that can actually happen, when running the DOM-Inspector we get:
> NS_ENSURE_TRUE(doc) failed: file /Users/ckerschb/Documents/moz/mc/parser/htmlparser/nsExpatDriver.cpp, line 800

We have two options:
(1) We could introduce yet another if-condition and either create NS_NewChannel(doc) or NS_NewChannel(nullprincipal), or
(2) we query the principal of the document (if there is one) and default to using the Nullprincipal otherwise.

I am not sure what the better option is. Within this patch I implement (2) since it reduces a little bit of complexity on the callsite. (1) on the other hand allows to store more accurate information within the loadInfo. Up to you, I don't feel strongly!

Btw, Firegestures within Bug 1226869 faces the same problem [1] and will be fixed with this patch as well.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1226869#c21

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, so apparently our RDF parser drives expat directly, not via the XML content sink.  So the nsIExpatSink in that case is an RDFContentSinkImpl and this always returns null from GetTarget().

txStylesheetSink is in the same situation, but presumably people don't normally load DTDs from those.

The other two implementations of nsIExpatSink are the XML and XUL content sinks, and both generally have a document to work with.

So what I think would be cleaner here in theory is to change nsIExpatSink to just have a getter returning a principal directly.  

In practice, I can't tell when txStylesheetSink is used, and have low confidence in RDFContentSinkImpl being only used for system-ish stuff, so presumably using a nullprincipal for both would be OK if it allows loading the stuff we care about.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8693987 [details] [diff] [review]
bug_1227554_expatdriver_default_nullprincipal.patch

r=me, but please:

  loadingPrincipal = nsNullPrincipal::Create();

and if that returns null (which it can, just like do_CreateInstance can), please just bail out.

Comment 12

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1227554_expatdriver_default_nullprincipal.patch

Thanks Boris - let's take this patch as a fix so addons work again on Nightly!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57d9f7884977

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57d9f7884977

Comment 15

[Philip Chee]

Thank you Christoph!