Loading file from local disk crashes browser with Zotero 3.0.8 installed

VERIFIED FIXED in Firefox 20

Status

()

--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: gerv, Assigned: bzbarsky)

Tracking

({crash, regression, testcase})

20 Branch
mozilla20
crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox20 verified)

Details

(crash signature)

Attachments

(2 attachments)

Created attachment 689994 [details]
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
Severity: normal → critical
Crash Signature: [@ nsDOMParser::InitInternal(nsISupports*, nsIPrincipal*, nsIURI*, nsIURI*) ]
Keywords: crash
(Assignee)

Comment 1

6 years ago
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
tracking-firefox20: --- → ?
Target Milestone: --- → mozilla20
(Assignee)

Comment 2

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

Updated

6 years ago
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?
(Assignee)

Comment 4

6 years ago
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.  :(

Updated

6 years ago
status-firefox20: --- → affected
Version: unspecified → 20 Branch

Updated

6 years ago
Keywords: testcase
I can binary-chop my extensions list. Give me a little time.

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

Updated

6 years ago
Crash Signature: [@ nsDOMParser::InitInternal(nsISupports*, nsIPrincipal*, nsIURI*, nsIURI*) ] → [@ nsDOMParser::InitInternal(nsISupports*, nsIPrincipal*, nsIURI*, nsIURI*)]
(Assignee)

Comment 7

6 years ago
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?
(Assignee)

Comment 9

6 years ago
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.

Gerv
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 "javascript.options.xml.chrome" 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["@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()...
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.
Created attachment 690683 [details] [diff] [review]
Null-check the window before jumping.
Attachment #690683 - Flags: review?(VYV03354)
(Assignee)

Updated

6 years ago
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.

Gerv
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...
http://hg.mozilla.org/integration/mozilla-inbound/rev/5ed9e56c7a5d
Component: HTML: Parser → DOM
Flags: in-testsuite?
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/5ed9e56c7a5d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
status-firefox20: affected → fixed
tracking-firefox20: ? → ---
Reproduced on nightly 2012-12-07.
Verified fixed FF 20b1 Win 7, Ubuntu 12.04 and Mac OS X 10.7.5
Status: RESOLVED → VERIFIED
status-firefox20: fixed → verified
QA Contact: paul.silaghi
You need to log in before you can comment on or make changes to this bug.