326279 - 1.0.8 builds fail to launch with pegged cpu usage

Status: VERIFIED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Tracy Walker [:tracy]]

seen on nightly builds for 2006-02-07

Windows build installs and launches migration (which is missing no migration option), cancel out.  Firefox launches as a tiny window flickering in upper left corner of screen.  cpu usage is pegged and firefox.exe is bouncing around the runnign process list.  I had to hard reboot my machine to stop it.

Marcia was seeing similar behavior on the mac build; once launched it was just bouncing in the dock. She also was forced to reboot her machine.

Marcia also said that yesterdays builds do not have this problem.

Comment 1

[Marcia Knous [:marcia]]

We could use some help narrowing down which checkin caused this regression.

Comment 2

[Marcia Knous [:marcia]]

nominating for 1.0.8

Comment 3

[Marcia Knous [:marcia]]

This affects the Mozilla 1.7.13 build as well. During triage we decided not to file a separate bug. I tested Mozilla with an existing profile and it came up with a blank profile manager (none of my existing profiles were listed).

Comment 4

[Mike Shaver (:shaver emeritus)]

If XCode is installed, the "Shark" profiler will let you see where Firefox is spending its time, and that would help us narrow things down a fair bit, I suspect.  (Mac builds always have their symbols in them, which is handy for this.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Local backout makes me pretty sure that this is a regression from bug 316589...  With that patch I get things like:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Could not convert JavaScript argument"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: file:///home/bzbarsky/mozilla/aviary/obj-firefox/dist/bin/components/nsExtensionManager.js :: nsExtensionManager__finishOperations :: line 1944"  data: no]
************************************************************

during attempts to start up (that's calling win.close(); there are other such exceptions, all involving window objects).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Something that might help

So the problem is that we need to create objects that legitimately (or whatever passes for it) don't have an intrinsic principal before the window is brought up (on the safe context). This patch allows those objects to actually be created, which fixes the only aspects of this bug that I could reproduce (no profiles in the profile manager). I *think* this patch should be safe, since every object that script deals with should either be parented by a window or a backstage pass, both of which have principals.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211098 [details] [diff] [review]
Something that might help

Yeah, looks reasonable to me.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 211098 [details] [diff] [review]
Something that might help

Should I check this in tonight?

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 211098 [details] [diff] [review]
Something that might help

Sorry I didn't catch this -- findObjectPrincipals is infallible, if it returns null that means "no principals for this object", not "error".  So it was never right to return NULL on that account from js_CloneFunctionObject.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I'm not really sure that I agree. We treat null prinicpals in eval as having no privileges at all, and I don't feel that js_CloneFunction really wants to behave differently. When we're running on the safe context, I think we might want to consider nulling out the runtime's (sorry, context's on the 1.7 branch) findObjectPrincipals hook or something.

I've checked the patch in to make the branch usuable again.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> I'm not really sure that I agree. We treat null prinicpals in eval as having no
> privileges at all, and I don't feel that js_CloneFunction really wants to
> behave differently. When we're running on the safe context, I think we might

Sorry, this is what Boris was trying to say to me last night. The problem, of course, isn't that we're creating objects on context 1 and trying to use them on context 2, it's that the two contexts disagree about their security model, and objects coming off of the safe context don't hold the same invariants as any other context. So I guess this patch is as good as it's going to get for now.

Comment 12

[Brendan Eich [:brendan]]

Blake talked sense into me:

1.  The branch has cx->findObjectPrincipals, the trunk rt->fOP.
2.  We fixed old null-principal-means-no-checks design flaws in SpiderMonkey by requiring non-null if findObjectPrincipals is non-null.  That's a good policy still, but the branch obviously violates it.

We could add a per-object flag of some kind to say it's ok to have non-null cx->fOP but it's allowed to return null without failing access checks, but that would be so wrong I won't even start.  Blake brought up the old "non-principal" idea:

3.  Don't fail, instead use the non-principal, which is unordered (via subsumes) w.r.t. all other principals including itself, and which does not equal itself.

Sounds like work.  OTOH, I think we want the non-principal on the trunk too.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

So if the QueryInterface() function here had the non-principal, would the QI call still be possible?

If yes, then I think we should do just that.  I've had some other thoughts on non-principal; want me to just file a bug with a thought-dump in it?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> So if the QueryInterface() function here had the non-principal, would the QI
> call still be possible?

Blake can say for sure, but I hope so.  Otherwise, we would have to add some flag (one bit is all we need) to distinguish this "good" (but blecherous) case from bad (as in exploit) ones.

> If yes, then I think we should do just that.  I've had some other thoughts on
> non-principal; want me to just file a bug with a thought-dump in it?

Sure, please do.  Cite the doc-comment in caps/idl/nsIPrincipal.idl in the URL if you buy it.  Thanks,

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

Filed bug 326506

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #14)
> Sure, please do.  Cite the doc-comment in caps/idl/nsIPrincipal.idl in the URL
> if you buy it.  Thanks,

Blake argues we want the non-principal to == itself and be < all other principals, or the QueryInterface call will fail.

My doc-comment wanted the non-principal to be unordered and not equal to itself, like IEEE754 NaN.

The difference matters if we use the non-principal to make two different roach motels, put valuables in them, and they ever manage (through some other bug) to become connected.  We wanted something like this for certain javascript: URL cases, but perhaps we can fix those by fixing the "track URI origin" bug instead.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note that I talked about that question at some length in bug 326506.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> Note that I talked about that question at some length in bug 326506.

Sorry, stale buffer in a textarea and I hit submit a day too late, before reading bugmail.  Wanted that buffer committed!

/be

Comment 19

[Tracy Walker [:tracy]]

verified with:
Windows:
Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215
Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215
Firefox/1.0.8