Last Comment Bug 672971 - can we remove SetSafeJSContext and its single caller in nsAppShellService?
: can we remove SetSafeJSContext and its single caller in nsAppShellService?
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Luke Wagner [:luke]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-20 15:31 PDT by Luke Wagner [:luke]
Modified: 2011-08-01 07:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm SetSafeJSContext (10.17 KB, patch)
2011-07-20 15:31 PDT, Luke Wagner [:luke]
bzbarsky: review+
Details | Diff | Splinter Review
fix workers (3.29 KB, patch)
2011-07-27 16:17 PDT, Luke Wagner [:luke]
luke: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-07-20 15:31:32 PDT
Created attachment 547278 [details] [diff] [review]
rm SetSafeJSContext

Currently, there is this really strange encapsulation-breaking method whereby nsAppShellService sets the safe JS context for the main thread to the hidden window's context.  mrbkap said this is stuff is from long ago and I should probably ask bz.  The attached patch (which just removes it) is green on try.

SetSafeJSContext isn't thing in the world, but it certainly confounds reasoning about the context stack and safe JS contexts.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-07-20 20:34:51 PDT
Hrm...  I thought we used to have code that in fact did depend on the safe context being the hidden window (or more importantly having an associated nsIScriptContext and the like), but maybe that's gone now.

With this change, is the safe context just some synthetic jscontext?

ccing jst too, because he may know more about this than I do (which would not be hard!).
Comment 2 Luke Wagner [:luke] 2011-07-21 08:17:56 PDT
(In reply to comment #1)
> With this change, is the safe context just some synthetic jscontext?

Yes, created by XPCJSContextStack::GetSafeJSContext.  It has null principals and some faked-up global object.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-07-21 08:34:18 PDT
Comment on attachment 547278 [details] [diff] [review]
rm SetSafeJSContext

Well, let's give this a shot.
Comment 4 Luke Wagner [:luke] 2011-07-21 08:39:35 PDT
I'll have wait a bit, one of the other uses of SetSafeJSContext is dom/src/threads/nsDOMThreadService.cpp which seems to have been given reprieve for the time being.
Comment 5 Luke Wagner [:luke] 2011-07-27 16:17:27 PDT
Created attachment 548956 [details] [diff] [review]
fix workers

Incidentally, this change did break new web workers.  Easy fix, though.  Over the shoulder r+ from Ben.
Comment 6 Marco Bonardo [::mak] 2011-08-01 07:48:16 PDT
http://hg.mozilla.org/mozilla-central/rev/7487eb60b55c

Note You need to log in before you can comment on or make changes to this bug.