Closed Bug 819588 Opened 10 years ago Closed 10 years ago

Loading file from local disk crashes browser with Zotero 3.0.8 installed


(Core :: DOM: Core & HTML, defect)

20 Branch
Not set



Tracking Status
firefox20 --- verified


(Reporter: gerv, Assigned: bzbarsky)



(Keywords: crash, regression, testcase)

Crash Data


(2 files)

Nightly 20.0a1, 2012-12-07 on Ubuntu 12.10. The attached file reproducibly crashes the browser! Example crash reports:


Severity: normal → critical
Crash Signature: [@ nsDOMParser::InitInternal(nsISupports*, nsIPrincipal*, nsIURI*, nsIURI*) ]
Keywords: crash
Probably a regression from bug 816410?

Any extensions involved?  This is crashing in DOMParser, which we wouldn't normally be running I don't think.
Assignee: nobody → VYV03354
Blocks: 816410
Target Milestone: --- → mozilla20
Based on the crash-stats data, _lots_ of extensions installed.  Gerv, do you have time to figure out which one is involved?
Keywords: regression
It seems that it's dereferencing the null domParser. Is it possible while the new operator is infallible? Or cycle-collection refcount is doing some magic?
We shouldn't have a null DOMParser here with infallible allocation, no.  Note that the crash-stats stack tops are useless because of pgo, by the way; there's a bunch of inlining going on there.  :(
Version: unspecified → 20 Branch
Keywords: testcase
I can binary-chop my extensions list. Give me a little time.

The problem is Zotero 3.0.8. I can report the bug to them if you like, but presumably crashing is unacceptable behaviour from us whatever it does...

Crash Signature: [@ nsDOMParser::InitInternal(nsISupports*, nsIPrincipal*, nsIURI*, nsIURI*) ] → [@ nsDOMParser::InitInternal(nsISupports*, nsIPrincipal*, nsIURI*, nsIURI*)]
Sure.  I'm just looking for a way to reproduce the crash.

So in a current debug build I installed Zotero 3.0.8 and loaded the testcase in this bug (directly from Bugzilla, not from local file), and no crash.  But I'm on 64-bit Mac in case that matters...

Gerv, I assume you can crash by just loading the Bugzilla attachment?  Any idea whether I need to do something with Zotero to trigger this crash?
I couldn't reproduce on Windows either. GCC optimizer bug?
Hmm.  The platform/OS says All....  Let me try on Linux.
Ah. I tried loading the file from the attachment, and it failed to crash. It seems this bug is not what I thought; loading any file from local disk causes the crash! (I spent all that work chopping and chopping to make a reduced test case...!)

Specifically, I loaded "file:///tmp/font.html". It also works when loaded from a bookmark, as I was doing that before. But now I find that loading any of 4 or 5 different files from local disk all lead to the crash.

Summary: File containing only string "<font>" crashes browser → Loading file from local disk crashes browser with Zotero 3.0.8 installed
Ah, there we go.  Now I get a crash.

We're landing in nsDOMParser::InitInternal with aOwner being a BackstagePass, not a window.  So QI to nsPIDOMWindow fails, but the null-check there is testing "aOwner", not "window", so we crash.

Masatoshi Kimura, I assume we should just null-check "window" instead?
Why nsDOMParser::Initialize didn't crash before bug 816410? Did nsIJSInitializer get the aOwner in a different way?
Hrm.  Yes, that's entirely possible...

I have to admit I was a little surprised to see a backstage pass here.  I can look into that more closely, but it'll be a few hours.
I locally backed out WebIDL patches and tested. nsDOMParser::Initialize wasn't even called. It looks like Zotero uses contract to create DOMParser.
The Zotero code actually does a ctor call, and if that throws uses the contract bit.
Note to self.  Starting today, bug 788290 landed, breaking this addon completely.  Need to flip "" to true to reproduce the bug now.
OK, so the current crash happens with a stack like so:

#5  0x00000001030409e4 in nsDOMParser::Constructor (aOwner=0x10d46eca8, rv=@0x7fff5fbce938) at nsDOMParser.cpp:396
#6  0x0000000104b06724 in mozilla::dom::DOMParserBinding::_constructor (cx=0x100554d50, argc=0, vp=0x10e587a20) at DOMParserBinding.cpp:500
#7  0x0000000104c03d03 in mozilla::dom::Constructor (cx=0x100554d50, argc=0, vp=0x10e587a20) at BindingUtils.cpp:210

and the top JS stack frame is:

0 anonymous() ["chrome://zotero/content/xpcom/translation/translate.js":2108]
    this = [object Object]

which has code that looks like this:

  "parseDOMXML":function(input, charset, size) {
    try {
      var dp = new DOMParser();
    } catch(e) {
      try {
        var dp = Components.classes[";1"]
      } catch(e) {
        throw new Error("DOMParser not supported");

etc.  Line 2108 is the "new DOMParser()" call.

The cx->global coming into this code is definitely the BackstagePass.  All the WebIDL code does is this:

      rv = xpc_qsUnwrapArg<nsISupports>(cx, val, &global, &globalRef.ptr, &val);

which works in this case, because there is in fact an nsISupports there.

On the other hand, the old code went through BaseStubConstructor in nsDOMClassInfo.cpp, which never passed anything other than an nsPIDOMWindow to Initialize().  Which explains why it worked there: either it actually had a window to work with and passed it in, or it just failed out from the constructor call before calling Initialize()...
Note also that in the old setup the BaseStubConstructor got a mWeakOwner passed to it explicitly from the nsDOMConstructor, whereas in our new setup we basically use whatever the global of the compartment the ctor is called in is.
Probably old constructor failed due to lacking of the nsPIDOMWindow, then Zotero falled back to the contract pass.
Given the analysis, I think your comment #11 is correct. We can just check "window" here (ans fail in this case).
So yeah, before we weren't even calling into BaseStubConstructor; we were just taking the createInstance codepath without ever getting that far.  We weren't even getting into nsDOMConstructor::Construct, at least on aurora.
Attachment #690683 - Flags: review?(VYV03354)
Assignee: VYV03354 → bzbarsky
Whiteboard: [need review]
When all the smoke clears, let me know if I also need to send a bug report to Zotero.

Attachment #690683 - Flags: review?(VYV03354) → review+
Gerv, you don't need to send them a bug report for this bug; it was purely our problem.

Bug 820237, on the other hand, needs to be fixed on their end...
Component: HTML: Parser → DOM
Flags: in-testsuite?
Whiteboard: [need review]
Closed: 10 years ago
Resolution: --- → FIXED
Reproduced on nightly 2012-12-07.
Verified fixed FF 20b1 Win 7, Ubuntu 12.04 and Mac OS X 10.7.5
QA Contact: paul.silaghi
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.