819588 - Loading file from local disk crashes browser with Zotero 3.0.8 installed

Status: VERIFIED FIXED

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

Description

[Gervase Markham [:gerv]]

Attachment: Very simple file; crashes Nightly

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

bp-ae2f659e-5437-4f99-9236-59f7a2121208
bp-dba524f4-e04d-4374-a23d-59aad2121208
bp-4603b513-0611-4bf9-98f6-7fe5f2121208

Gerv

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Based on the crash-stats data, _lots_ of extensions installed.  Gerv, do you have time to figure out which one is involved?

Comment 3

[Masatoshi Kimura [:emk]]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 5

[Gervase Markham [:gerv]]

I can binary-chop my extensions list. Give me a little time.

Gerv

Comment 6

[Gervase Markham [:gerv]]

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...

Gerv

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Masatoshi Kimura [:emk]]

I couldn't reproduce on Windows either. GCC optimizer bug?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  The platform/OS says All....  Let me try on Linux.

Comment 10

[Gervase Markham [:gerv]]

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.

Gerv

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Masatoshi Kimura [:emk]]

Why nsDOMParser::Initialize didn't crash before bug 816410? Did nsIJSInitializer get the aOwner in a different way?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Masatoshi Kimura [:emk]]

I locally backed out WebIDL patches and tested. nsDOMParser::Initialize wasn't even called. It looks like Zotero uses contract to create DOMParser.

Comment 15

[Boris Zbarsky [:bzbarsky]]

The Zotero code actually does a ctor call, and if that throws uses the contract bit.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note to self.  Starting today, bug 788290 landed, breaking this addon completely.  Need to flip "javascript.options.xml.chrome" to true to reproduce the bug now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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["@mozilla.org/xmlextras/domparser;1"]
           .createInstance(Components.interfaces.nsIDOMParser);
      } 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()...

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[Masatoshi Kimura [:emk]]

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).

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Null-check the window before jumping.

Comment 22

[Gervase Markham [:gerv]]

When all the smoke clears, let me know if I also need to send a bug report to Zotero.

Gerv

Comment 23

[Boris Zbarsky [:bzbarsky]]

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...

Comment 24

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5ed9e56c7a5d

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ed9e56c7a5d

Comment 26

[Paul Silaghi, QA [:pauly]]

Reproduced on nightly 2012-12-07.
Verified fixed FF 20b1 Win 7, Ubuntu 12.04 and Mac OS X 10.7.5