789059 - crash in nsACString_internal::EqualsASCII (or in debug builds: "Assertion failure: false (All IPDL URIs must be serializable or an allowed scheme!), at URIUtils.cpp:83")

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Cork]

This bug was filed from the Socorro interface and is 
report bp-98f347d3-b7cf-4ace-93b7-24ddb2120906 .
============================================================= 

STR:
 1. Start firefox with a new profile
 2. Goto moz-icon://stock/gtk-refresh?size=menu

expected:
You should see the gtk theme refresh icon.

actually:
Forefox crashes.

Comment 1

[Cork]

Should prob have spelled it out:
The first bad revision is:
changeset:   103215:c813eeb62b92
user:        Ben Turner <[e-mail]>
data:        Thu Aug 23 12:33:46 2012 -0700
summary:     Bug 784726 - 'Remove old IPC:URI'. r=cjones+khuey.

Comment 2

[Cork]

For the record i can't even open this bug in firefox without crashing for some reason.

Comment 3

[Cork]

Gnome shell or gtk3 seams to be a dependency for that crash.

My old gnome2 comp doens't crash on that url.

Ubuntu 10.10 - works
Linux mint 11(gnome shell) - crashes
archlinux with gnome shell - crashes

Comment 4

[Daniel Holbert [:dholbert]]

My firefox session has been hanging* or crashing within ~2 minutes of starting this morning (and the crashes have been in nsACString_internal::EqualsASCII inside some URI-hadnling stuff, so I'm pretty sure it's this bug)

Crash reports:
bp-ce397a32-d8d5-4a00-9f8f-a5ca02120906
bp-a7c4b007-61f7-4d4d-98e9-a0ebb2120906

*The hangs have been serious enough that "kill" and "kill -11" won't fix it -- only "kill -9"

I have gnome-shell installed, but I'm logged in with 2d "gnome classic" at the moment.

Comment 5

[Daniel Holbert [:dholbert]]

(and I'm running Ubuntu 12.10 alpha)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Cork from comment #2)
> For the record i can't even open this bug in firefox without crashing for
> some reason.

Same here. (Using Opera to view this page at the moment).  --> Dogfood keyword.

(Not sure why I'm just now hitting this today, when the cset in question landed a few weeks back, per comment 1)

In my debug build, using the moz-icon URI from comment 0, I abort with this assertion-failure:

Assertion failure: false (All IPDL URIs must be serializable or an allowed scheme!), at ../../../mozilla/ipc/glue/URIUtils.cpp:83

from this code:
>   if (!allowed) {
>     MOZ_NOT_REACHED("All IPDL URIs must be serializable or an allowed "
>                     "scheme!");
>   }

That boolean "allowed" is set from us iterating through a list of whitelisted schemes and checking for a match.  The scheme we've got is "moz-icon"; the whitelist (kGenericURIAllowedSchemes) just includes "about:", "javascript:", and "javascript".

Comment 7

[Daniel Holbert [:dholbert]]

Just to verify, I ran mozregression, and I can confirm that the range includes the cset from comment 1. 
Last good nightly: 2012-08-23
First bad nightly: 2012-08-24
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5650196a8c7d&tochange=1c0ac073dc65

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: backtrace of assertion failure

Here's the gdb backtrace when I hit the MOZ_NOT_REACHED() noted in comment 6.

Comment 9

[Daniel Holbert [:dholbert]]

It looks like most URIs get serialized in the first block of SerializeURI:
>  nsCOMPtr<nsIIPCSerializableURI> serializable = do_QueryInterface(aURI);
>  if (serializable) {
>    serializable->Serialize(aParams);
>    if (aParams.type() == URIParams::T__None) {
>      MOZ_NOT_REACHED("Serialize failed!");
>    }
>    return;
>  }
but in this case, "serializable" is null, so I take it aURI (an instance of nsMozIconURI) doesn't support the nsIIPCSerializableURI interface.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: hackaround: bail out of History::NotifyVisited

I made this hackaround for myself -- it just returns early from History::NotifyVisited() if it's called with a nsIMozIconURI.  Posting it here in case anyone else finds it useful.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Passing this to Chris Jones who is the module owner for IPC to determine what can be done here or who can take this on.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This will probably end up being my bug. There are a few things here that suck:

1. History::NotifyVisited is trying to serialize a URI to send to... nothing. We shouldn't have any child processes on desktop linux that would receive such a message, so this is just wasted work anyway.

2. History::NotifyVisited probably shouldn't care about icon urls anyway. What good does it do us to store that in the places db?

3. We need to figure out whether or not moz-icon URIs are serializable. If they are then we should just do the work to make them support nsIIPCSerializableURI. Otherwise we need to handle this case more gracefully.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks to whoever is sending the nag emails, this fell off my plate.

bent, yeah, this seems in your wheelhouse.  This isn't a core IPC bug.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks for resetting that, bz.

Comment 15

[Alex Keybl [:akeybl]]

(In reply to ben turner [:bent] from comment #12)
> This will probably end up being my bug. There are a few things here that
> suck:

If we don't come up with a low risk fix soon, we will likely untrack for release. Although this is a regression, it's not clear that the user impact would be significant (this is not a top crasher).

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 658958 [details] [diff] [review]
hackaround: bail out of History::NotifyVisited

(In reply to Alex Keybl [:akeybl] from comment #15)
> If we don't come up with a low risk fix soon, we will likely untrack for
> release.

My attached patch is a low-risk workaround, if not a fix. bent, what do you think?

(This makes us skip over saving "visited" information for moz-icon URIs, instead of crashing.  It adds one QI, so there's a little overhead, but that's probably negligible given that we're doing URI-serialization & sqlite operations (I think?) alongside this.)

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Workaround, v1

If we're going to just hack something in to avoid the crash then I think fixing only (1) from comment 12 is best.

We still need to fix (2) and (3) in comment 12 but in followups I guess.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> My attached patch is a low-risk workaround, if not a fix. bent, what do you
> think?

dholbert, thanks for your investigation and patch. Would you mind testing this one?

Comment 19

[Daniel Holbert [:dholbert]]

Sure -- just tried it, and it does fix the crash for me.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/mozilla-central/rev/84beb8eebe78

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 667989 [details] [diff] [review]
Workaround, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 784726
User impact if declined: Linux users crash or hang in weird ways.
Testing completed (on m-c, etc.): m-c, dholbert has verified manually
Risk to taking this patch (and alternatives if risky): This patch is quite safe.
String or UUID changes made by this patch: None

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 667989 [details] [diff] [review]
Workaround, v1

[Triage Comment]
Approving for Aurora 17 given this is a new reproducible regression, with a low risk fix. Please land early Monday to make the next merge.

Comment 24

[Phil Ringnalda (:philor)]

Beat the morning rush with https://hg.mozilla.org/releases/mozilla-aurora/rev/0279304f2265, since this was the only one of today's (public) approvals with an attached patch that actually applies to aurora.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Filed bug 801882 to figure out the rest of our story here.

Comment 26

[Cork]

Verified fixed with:
Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/19.0 Firefox/19.0 (2012-10-15 nightly)
Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/18.0 Firefox/18.0 (2012-10-16 beta 1)
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 (2012-10-10 aurora 2)

Checked against:
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 (2012-10-05 aurora 2) [crashed]

Comment 27

[u279076]

Thanks Cork.