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

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[The 8472]

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

[Loic]

Do you have STR for the crash?

Comment 2

[The 8472]

Attachment: 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

[Loic]

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

Comment 4

[The 8472]

did you restart after installing the xpi? did you use the browser toolbox (not the inline debugger)?

Comment 5

[Loic]

Done, I got the crash:
https://crash-stats.mozilla.com/report/index/abf35e9f-6fbb-41e7-bbd3-8f99d2150825

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks like XPConnect on the stack, perhaps that group may know more.

Comment 7

[Loic]

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

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Loic]

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

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[u408661]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[u408661]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[u408661]

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

Comment 18

[u408661]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea5200f79451

Comment 19

[u408661]

(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

Comment 20

[Patrick McManus [:mcmanus]]

dont forget to send it to 42

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58e036ba3d66

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/58e036ba3d66

Comment 23

[u408661]

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.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8662014 [details] [diff] [review]
patch

Fix a crash, taking it.

Comment 25

[u408661]

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.

Comment 26

[Carsten Book [:Tomcat]]

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 ?

Comment 27

[Sylvestre Ledru [:Sylvestre]]

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

Comment 28

[Jorge Villalobos [:jorgev] (he/him)]

Looks good to me.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0439874d3e4