453642 - Safe mode should disable JIT

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benjamin Smedberg]

Having safe-mode on should disable JIT. This will probably involve moving nsIXULRuntime around a bit so that it is available to the DOM code.

Comment 1

[Dave Townsend [:mossop]]

bug 342333 has a patch that might be useful for moving nsIXULRuntime

Comment 2

[Jason Orendorff [:jorendorff]]

If this is a reasonably small amount of work, it might be block-worthy.  Nominating.

Comment 3

[Benjamin Smedberg]

Attachment: Disable JIT when safe mode is on, rev. 1

Patch should work, haven't really tested it yet.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 356067 [details] [diff] [review]
Disable JIT when safe mode is on, rev. 1

- In xpcom/system/Makefile.in

 XPIDLSRCS = \
         nsIXULAppInfo.idl \
+	nsIXULRuntime.idl \
         nsIGConfService.idl \

Fix the inconsistent indentation?

r+sr=jst

Comment 5

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/3b9d2ef2e4a8 (with the wrong bug# in the commit message, argh!)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I don't understand why this is useful. If someone is crashing due to content JIT, there are easier and more obvious workarounds than "start in safe mode to disable it".

Comment 7

[u88484]

This bug was a waste of time and added code bloat.  The time and code could have been better spent fixing 333808 which would have and will one day, have greater effect then this bug.  At least with JIT, it can be disabled through about:config unlike userChrome and userContent.

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #6)
> I don't understand why this is useful. If someone is crashing due to content
> JIT, there are easier and more obvious workarounds than "start in safe mode to
> disable it".

By that rationale safe mode is pointless since there are specific ways to disable everything it disables. The point is that safe mode is recommended as the first thing to try, at which point you can then start narrowing down whether it might be an extension problem, or JIT, or whatever...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> By that rationale safe mode is pointless since there are specific ways to
> disable everything it disables.

That's not true - extensions and plugins can make the browser unusable enough that disabling them is not possible (i.e. a crash on startup). Content JIT crashes, on the other hand, can only be triggered by web pages that you visit, and we have protection against pages continuously crashing you on startup due to session restore (we avoid automatically restoring the session the second time around).

Comment 10

[u88484]

(In reply to comment #9)
> (In reply to comment #8)
> > By that rationale safe mode is pointless since there are specific ways to
> > disable everything it disables.
> 
> That's not true - extensions and plugins can make the browser unusable enough
> that disabling them is not possible (i.e. a crash on startup). Content JIT
> crashes, on the other hand, can only be triggered by web pages that you visit,
> and we have protection against pages continuously crashing you on startup due
> to session restore (we avoid automatically restoring the session the second
> time around).

Great point.  By that comment alone, that should be enough reason not to over code things needlessly.  Hell, we might as well just disable the whole javascript engine in safe mode if we are worried about crashes on webpages.  This bug just seems like a bandaid in that it is believed users are going to experience a load of crashes with JIT.  If that is the case, set JIT to FALSE until it is felt the code is stable enough for the default to be TRUE.

I still feel that fixing bug 333808 would have greater use.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> I still feel that fixing bug 333808 would have greater use.

It's actually worth noting that pretty much all of the changes that went into this patch would have been necessary in order to fix bug 333808 anyway. In fact now they are done fixing bug 333808 is not far off copying the safe mode check to the place I indicated in that bug.

Comment 12

[Jason Orendorff [:jorendorff]]

> This bug was a waste of time and added code bloat.

Regardless of whether this is true, your tone seems rude to me.  Please keep it friendly.

Comment 13

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/961f1ed53daa

Comment 14

[Dão Gottwald [:dao]]

This also makes it harder to figure out if there's a problem with an extension and whether or not it involves chrome jit. I.e. you can't use safe mode to quickly disable all add-ons -- the problem might be gone because jit is disabled, because an extension is disabled, or both.

For example, see the confusion in http://forums.mozillazine.org/viewtopic.php?p=5774025#p5774025 -- trying safe mode was entirely redundant there, as the problem was already known to be tied to javascript.options.jit.chrome=true. To see that there's no extension involved, the user had to manually disable all extensions.

Comment 15

[Tony Chung [:tchung]]

On Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090305 Minefield/3.2a1pre

Hi, is this fixed?  i'm trying to verify this, but i'm unsure if i'm seeing expected behavior or not testing properly.

STR: 
1) login to build above, set jit.content & jit.chrome = true
2) Save & Close browser, restart in safemode
3) open about:config, search on jit
4) jit.content 8* jit.chrome still shows = true.

Shouldn't i be expecting both to = false when opening in safemode?

Comment 16

[Benjamin Smedberg]

tchung, no: this doesn't alter the pref values at all: it only alters whether we read and apply those values, or just always disable JIT.

Comment 17

[Tony Chung [:tchung]]

in that case, is there a way i can test this as a user?   Where is the testcase for this patch?

Comment 18

[Dão Gottwald [:dao]]

You can run a JS benchmark. For chrome jit, I guess you'll have to find a crash that depends on it.

Comment 19

[Tony Chung [:tchung]]

(In reply to comment #18)
> You can run a JS benchmark. For chrome jit, I guess you'll have to find a crash
> that depends on it.

Dao, thanks for the tip.  Using the crashing URL testcase in bug 481302, i can verify that safemode does indeed disable jit.chrome.  

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090304 Firefox/3.1b3
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090305 Minefield/3.2a1pre