Debugging/inspecting a nsiHttpChannel crashes browser in nsJSID::NewID(nsID const&)

RESOLVED FIXED in Firefox 42

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bugzilla.mozilla.org, Assigned: u408661)

Tracking

({crash, regression})

42 Branch
mozilla43
x86_64
Windows 7
crash, regression
Points:
---

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ fixed, firefox43+ fixed)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Using the browser toolbox I was debugging a http-on-modify-request observer and had a nsiHttpChannel in the local scope. Upon inspecting it and trying to drill down on the loadGroup property of the channel the browser crashed.

crash id: bp-c1c8e520-9b1a-4c3d-a2b0-b622e2150825

Comment 1

3 years ago
Do you have STR for the crash?
(Reporter)

Comment 2

3 years ago
Created attachment 8652097 [details]
greasemonkey-2015.08.25.beta.xpi

see if this works for you:

- clean profile
- e10s off
- addon signature verification off
- install custom greasemonkey build (attached)
- open browser toolbox
- set a breakpoint on refererSetter.js line 33
- open a page to trigger the observer
- select channel from the local scope in the variables view
- try to open the "loadGroup" property

That's on nightly Build ID 	20150821030204

Comment 3

3 years ago
(In reply to The 8472 from comment #2)
> - set a breakpoint on refererSetter.js line 33

I don't see this js script in the debugger.
(Reporter)

Comment 4

3 years ago
did you restart after installing the xpi? did you use the browser toolbox (not the inline debugger)?
Looks like XPConnect on the stack, perhaps that group may know more.
Component: Developer Tools: Debugger → XPConnect
Product: Firefox → Core

Comment 7

3 years ago
It regressed:
good=2015-08-05
bad=2015-08-06
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f3b757156f69&tochange=07befc6f54e7
Suspected bug:
Andrew McCreight — Bug 1190436 - Part 2: Use an early return in XPCConvert::JSObject2NativeInterface. r=gabor
Andrew McCreight — Bug 1190436 - Part 1: Use more smart pointers in XPConnect. r=gabor
Blocks: 1190436
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsJSID::NewID(nsID const&) ]
status-firefox41: --- → unaffected
status-firefox42: --- → affected
status-firefox43: --- → affected
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
Ever confirmed: true
Flags: needinfo?(continuation)
Keywords: crash, regression
Summary: Debugging/inspecting a nsiHttpChannel crashes browser → Debugging/inspecting a nsiHttpChannel crashes browser in nsJSID::NewID(nsID const&)
Version: Trunk → 42 Branch
Assignee: nobody → continuation
Flags: needinfo?(continuation)
Any chance you could try bisecting across inbound using mozregression to narrow the range a bit? Thanks.

It isn't immediately clear to me how my patches could have caused this, though I do agree they are the most suspect in that range.
Flags: needinfo?(epinal99-bugzilla2)

Comment 9

3 years ago
m-i:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=579d50cc0ca7&tochange=91a3e2205388

I see:
Andrew McCreight — Bug 1155773 - Remove gotos from XPCConvert::NativeArray2JS(). r=bholley
Flags: needinfo?(epinal99-bugzilla2)
tracking-firefox42: ? → +
tracking-firefox43: ? → +
I can reproduce this locally. I'm still not sure what is the matter here. It looks like the nsID we're getting in XPCConvert::NativeData2JS() is bogus. I reverted bug 1155773 and bug 1190436, but that did not fix the crash.
I did my own mozregression which pointed to bug 1127618 (which matches up with the regression range from comment 9, but not the prior ones):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/69f2a0f7b8ad400e807a2c57424585c2b4ff9d31
I was able to confirm the regression using local builds.

That patch involves networking, and the reported problem seems to involve nsIHttpChannel so there's at least some relation.

Could you look into this regression, Nick? Thanks.
Assignee: continuation → nobody
Blocks: 1127618
No longer blocks: 1190436
Flags: needinfo?(hurley)
(Assignee)

Comment 12

3 years ago
Lest anyone think I haven't noticed this - I have. Unfortunately, while I can reproduce the crash, I'm currently no closer to finding out why the crash is occurring. My one thought so far (and this is a long shot!) is that the code to serialize an "attribute nsID" into script is somehow incorrect - my use added in bug 1127618 is the *only* use of that, so I have no others to compare with (any other script-accessible nsIDs are through an "attribute nsIDPtr"). I'm continuing to dig in to see if I can narrow down the problem more.
Flags: needinfo?(hurley)
Um...  Why are you using "attribute nsID"?  That's not supported from script at all, and will cause you bad times and corrupted memory.  See the comment at http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsrootidl.idl#57
Flags: needinfo?(hurley)
Oh, and the fact that xpidl didn't barf on that is a bug, at least assuming I believe the last sentence of that comment...
(Assignee)

Comment 15

3 years ago
Well, that would certainly explain the crash :) I don't see any reason for this to be accessible via script, so I'll just [notxpcom] it. (FWIW, I wanted 'attribute nsID' because I didn't want to have to bother manually freeing memory for the nsIDs.)

And yes, if that comment is supposed to be correct (aren't they all?! :P) then this does sound like an xpidl bug, as well.
Flags: needinfo?(hurley)
> I don't see any reason for this to be accessible via script

Ah, good.  Then let's notxpcom by all means.  And if that fixes the crash, please file another bug on xpidl.
(Assignee)

Comment 17

3 years ago
Created attachment 8662014 [details] [diff] [review]
patch

So I actually went with 'noscript' instead of 'notxpcom' (xpidl seemed to barf on 'notxpcom' by itself - maybe because the interface is labeled 'scriptable'?) but it has the effect we want. Interesting side note, I appear to have made all other instances of an 'attribute nsID' that I added to scriptable interfaces 'noscript' in the original patch, so obviously I at some point knew that was the right thing to do. I just didn't do it for nsILoadGroup, for some reason.
Attachment #8662014 - Flags: review?(mcmanus)
(Assignee)

Updated

3 years ago
Assignee: nobody → hurley
Status: NEW → ASSIGNED
(Assignee)

Comment 19

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #16)
> > I don't see any reason for this to be accessible via script
> 
> Ah, good.  Then let's notxpcom by all means.  And if that fixes the crash,
> please file another bug on xpidl.

Filed bug 1205419
Attachment #8662014 - Flags: review?(mcmanus) → review+
dont forget to send it to 42
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/58e036ba3d66
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 23

3 years ago
Comment on attachment 8662014 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: 1127618
[User impact if declined]: crashes when inspecting certain objects in browser toolbox
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: low
[String/UUID change made/needed]: nsILoadGroup UUID changed to make an attribute noscript. The UUID for this already changed in gecko 42, anyway.

Note: this may need to be approved for beta instead, if this doesn't get approved and landed on aurora before the next uplift.
Attachment #8662014 - Flags: approval-mozilla-aurora?
Comment on attachment 8662014 [details] [diff] [review]
patch

Fix a crash, taking it.
Attachment #8662014 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 25

3 years ago
Comment on attachment 8662014 [details] [diff] [review]
patch

(Copied from comment 23 - this didn't get approved in time for me to get this into aurora 42, so now it has to go into beta 42)

Approval Request Comment
[Feature/regressing bug #]: 1127618
[User impact if declined]: crashes when inspecting certain objects in browser toolbox
[Describe test coverage new/current, TreeHerder]: on m-c and m-a
[Risks and why]: low
[String/UUID change made/needed]: nsILoadGroup UUID changed to make an attribute noscript. The UUID for this already changed in gecko 42, anyway.
Attachment #8662014 - Flags: approval-mozilla-beta?
Attachment #8662014 - Flags: approval-mozilla-beta?
Attachment #8662014 - Flags: approval-mozilla-beta+
Attachment #8662014 - Flags: approval-mozilla-aurora-
Attachment #8662014 - Flags: approval-mozilla-aurora+
when applying this to beta i get:

remote: *** IDL file netwerk/base/nsILoadGroup.idl altered in this changeset***
remote: 
remote: Changes to IDL files in this repo require you to provide binary change approval in your top comment in the form of ba=... (or, more accurately, ba\S*=...)
remote: This is to ensure that UUID changes (or method changes missing corresponding UUID change) are caught early, before release.

is this expected ?
Flags: needinfo?(sledru)
Well, yes and no. The patch was expected to land in aurora and not in beta2...
Jorge, are you ok with this IDL change? Thanks
Flags: needinfo?(sledru) → needinfo?(jorge)
Looks good to me.
Flags: needinfo?(jorge)
You need to log in before you can comment on or make changes to this bug.