666713 - Dynamic analysis of chrome->content operations in Firefox

Status: NEW

(Core :: General, defect)

Description

[Benjamin Smedberg]

In order to scope content processes, we need a dynamic analysis of ways in which chrome touches content. We want to identify:

    non message-manager scripts
    which touch a "content object"
    or implement nsIWebProgressListener 

Definitions:

Non message-manager script is any chrome script that isn't run through nsIChromeFrameMessageManager.loadFrameScript.

Content objects are at least the following:

    Any DOM object (window, document, or node)
    Docshell or docshell derivative
    nsIWebProgress
    nsIDOMWindowUtils

Although it turns out that the nsIWebProgressListener bit may be unnecessary, because it's necessary to get a nsIWebProgress to attach a listener.

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP patch

This patch is close to reviewable I think.  Benjamin, could you verify that it captures what we want in the manner we want?

Example output when starting up Firefox and immediately quiting is here: http://people.mozilla.com/~adw/e10s-content-log.txt

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 541500 [details] [diff] [review]
WIP patch

Blake, could you check that I've logged all the right wrapper sites to support the goal in comment 0?

Comment 3

[Drew Willcoxon :adw]

Hmm, looks like I'm missing some wrapper creation sites:  there are some property accesses on wrappers that don't have corresponding previous creations.  Logging WrapperFactory::PrepareForWrapping right after the JSAutoEnterCompartment is created seems to capture them.

Comment 4

[Drew Willcoxon :adw]

Oh, it's probably obvious, but to get a clean log like the one in comment 1 you can grep stdout:

egrep '^@adw|^[0-9]+|^JavaScript stack is empty' | sed 's/^@adw //'

The @adws are only temporary of course, and I expect the printfs are too.

Comment 5

[Benjamin Smedberg]

Comment on attachment 541500 [details] [diff] [review]
WIP patch

I told drew that I couldn't provide much technical feedback. I do want to get this deployed onto mozilla-central in a way that debug builds at least can run it. This probably involves a little bit of fixing the printfs so that they can be sent to a logfile instead of stdout. After the most obvious pieces of Firefox are fixed, the noise will be significantly reduced and we can start taking logs from debug mochitest-* runs to get a good measure of the scope of remaining work for the Firefox frontend.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 541500 [details] [diff] [review]
WIP patch

Review of attachment 541500 [details] [diff] [review]:
-----------------------------------------------------------------

There's a couple of overarching cleanup things here: the XXXadw comments don't seem necessary to me, especially if this is going to be checked in. There's a block of commented-out code that could be removed (or debugged, I don't think we should crash there). Overall, this looks like a fine approach though. I have a bunch of nitpicks and small comments, though.

::: content/base/src/nsInProcessTabChildGlobal.cpp
@@ +324,5 @@
>    NS_ENSURE_SUCCESS(rv, false);
>  
> +  //XXXadw
> +  if (!JS_DefineProperty(cx, global, "__isFrameScript__",
> +                         BOOLEAN_TO_JSVAL(true), NULL, NULL, JSPROP_ENUMERATE))

I really don't like this. I'd much rather that instead of testing for this property in C++, you actually got the C++ object out and QI'd it.

::: js/src/xpconnect/src/xpcwrappednative.cpp
@@ +643,5 @@
>  }
>  
> +//XXXadw
> +static JSBool
> +isWrapperContentRelated(XPCWrappedNative* wrapper)

Nit corner: Start static functions like this with a capital letter.

Style in XPConnect-land is ugly, but is:

if(foo)
{
    bar;
}
else
{
    baz;
}

::: js/src/xpconnect/wrappers/Makefile.in
@@ +51,5 @@
>  	          CrossOriginWrapper.cpp \
>  		  FilteringWrapper.cpp \
>  		  XrayWrapper.cpp \
> +		  WrapperFactory.cpp \
> +		  contentLog.cpp

Another nit: mind making this ContentLog.cpp to fit in with the naming conventions for the rest of the directory?

::: js/src/xpconnect/wrappers/WrapperFactory.cpp
@@ +152,5 @@
>  
> +        //XXXadw I tried a logContentAccess here, but the output didn't make
> +        // sense, and it seemed to be called multiple times for the same objects
> +        // that the Rewrap one was but with various unhelpful values for
> +        // "object:".

Yeah, this is redundant with respect to the logContentAccess in Rewrap.

@@ +285,5 @@
> +            // is.  but which one(s) of the branches below is relevant?  seems
> +            // like the else branch, at least.
> +            //XXXadw i tried putting the shouldDumpJSInfo = true here rather
> +            // than below, but it didn't make a difference, not on startup at
> +            // least.

I don't really understand this comment. Under it is one case where the object being touched actually is a chrome object (the isSystem case) and the other two are content objects being used in chrome.

I think that the way things are set up right now, you're catching wrapper creation in a bunch of places. It would be a little cleaner to consolidate all of it into this function (and, in particular) this section of the code.

@@ +397,5 @@
>      // is relied on by XPCNativeWrapper.unwrap.
>      wrapperObj->setProxyExtra(js::ObjectValue(*xrayHolder));
> +
> +    //XXXadw
> +    if (shouldDumpJSInfo)

This will miss .wrappedJSObject uses since they will have returned early above. However, this omission is worked around by the logContentAccess in WaiveXrayAndWrap.

@@ +444,5 @@
>      if (obj->compartment() == cx->compartment) {
>          *vp = OBJECT_TO_JSVAL(obj);
> +
> +        //XXXadw i tried a logContentAccess here, but it doesn't seem to get
> +        // called.  a breakpoint in gdb confirms.

I also think that we could assert that the object in question is a chrome object, so you wouldn't want a logContentAccess.

@@ +453,5 @@
>      obj = WaiveXray(cx, obj);
>      if (!obj)
>          return false;
>  
> +    //XXXadw not sure if it's right to call logContentAccess here.

It is, but it would have been cleaner to do this in Rewrap.

::: js/src/xpconnect/wrappers/XrayWrapper.cpp
@@ +859,5 @@
> +    //XXXadw
> +    // I'm wondering about what kind of things get trapped here.  This line at
> +    // extensions-content.js:254 did:
> +    //   window.wrappedJSObject.__defineGetter__("InstallTrigger", function() {
> +    // The object that was printed was:

This is weird. I wouldn't expect this code to run for that line of JS code.

::: js/src/xpconnect/wrappers/contentLog.cpp
@@ +17,5 @@
> +using namespace xpc;
> +
> +
> +static void
> +log(const char *fmt="", ...)

Why the default parameter?

@@ +22,5 @@
> +{
> +  const char *uberFmt = "@adw %s\n";
> +  int uberAllesFmtLen = strlen(fmt) + strlen(uberFmt) - 2 + 1;
> +  char *uberAllesFmt =
> +    reinterpret_cast<char *>(NS_Alloc(sizeof(char) * uberAllesFmtLen));

Can you use nsPrintfCString here?

@@ +73,5 @@
> +
> +  // message manager script origin
> +  {
> +    JSAutoEnterCompartment ac;
> +    JSObject *scriptGlobal = JS_GetGlobalObject(cx);

I think you want JS_GetGlobalForScopeChain here. JS_GetGlobalObject(cx) gets you the global object of the context, which may or may not have anything to do with the currently-running code.

Comment 7

[Drew Willcoxon :adw]

Thanks Blake.  A couple of questions before I can finish a for-review patch:

(In reply to comment #6)
> There's a couple of overarching cleanup things here: the XXXadw comments
> don't seem necessary to me, especially if this is going to be checked in.

Sure, they're just breadcrumbs and thoughts while I'm working.

> ::: content/base/src/nsInProcessTabChildGlobal.cpp
> @@ +324,5 @@
> >    NS_ENSURE_SUCCESS(rv, false);
> >  
> > +  //XXXadw
> > +  if (!JS_DefineProperty(cx, global, "__isFrameScript__",
> > +                         BOOLEAN_TO_JSVAL(true), NULL, NULL, JSPROP_ENUMERATE))
> 
> I really don't like this. I'd much rather that instead of testing for this
> property in C++, you actually got the C++ object out and QI'd it.

Definitely a hack, but I don't know what you mean by "got the C++ object out".  I tried QI'ing the ptr returned by JS_GetContextPrivate(cx) to nsIFrameMessageManager, and that seems to work.  (Neat trick!)  Is that what you mean?

> @@ +397,5 @@
> >      // is relied on by XPCNativeWrapper.unwrap.
> >      wrapperObj->setProxyExtra(js::ObjectValue(*xrayHolder));
> > +
> > +    //XXXadw
> > +    if (shouldDumpJSInfo)
> 
> This will miss .wrappedJSObject uses since they will have returned early
> above. However, this omission is worked around by the logContentAccess in
> WaiveXrayAndWrap.

All the early returns in WrapperFactory::Rewrap look like error conditions to me.  Which one are you referring to?

Comment 8

[Drew Willcoxon :adw]

Attachment: for-review patch

This is ready for review, but I still have the two questions in comment 7.

Comment 9

[Drew Willcoxon :adw]

I'll do some mochitest suite runs to see how this affects its time to completion.  The 8 KB output stream buffer size may be too small (or large).  Identical stacks may need to be ignored to reduce the amount of I/O.  (Hmm, that would be nice for the human going through the log, too.)  Starting the browser, clicking a link or two, and opening the page info window yielded a 2 MB log. :\

Comment 10

[Drew Willcoxon :adw]

I started a mochitest-plain run around noon and when I left the office at 7:00 it was still going, and that's with my patch but with the logging turned off.  I wonder if it's so slow because I'm running it headless with xvfb.  Anyway, next week I'll try only mochitest-browser-chrome without my patch at all to get a baseline.

Comment 11

[Benjamin Smedberg]

Note that there is a case mismatch in ContentLog.cpp:

#include "contentLog.h"

must be #include "ContentLog.h"

Comment 12

[Drew Willcoxon :adw]

Yeah, thanks.  (I renamed that file at the last minute, and I'm not sure why it still successfully compiled...)  I'll batch that fix with whatever other review comments you and Blake have.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #7)
> Definitely a hack, but I don't know what you mean by "got the C++ object
> out".  I tried QI'ing the ptr returned by JS_GetContextPrivate(cx) to
> nsIFrameMessageManager, and that seems to work.  (Neat trick!)  Is that what
> you mean?

Instead of looking up the property on the object, you can take the JS object and call nsIXPConnect::GetNativeOfWrapper. If that QIs to nsIFrameMessageManager, then you have the right JS object.

> All the early returns in WrapperFactory::Rewrap look like error conditions
> to me.  Which one are you referring to?

There's a tricky one: if (!wrapperObj || !xrayHolder) return wrapperObj; might be a success condition if we're not creating an Xray wrapper.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 546034 [details] [diff] [review]
for-review patch

Review of attachment 546034 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me with the following comments addressed.

::: js/src/xpconnect/src/xpcwrappednative.cpp
@@ +649,5 @@
> +{
> +    XPCNativeSet* set = wrapper->GetSet();
> +    XPCNativeInterface** ifaces = set->GetInterfaceArray();
> +    PRUint16 ifacesLen = set->GetInterfaceCount();
> +    for (PRUint16 i = 0; i < ifacesLen; i++)

Uber-nit: (here and below) nix the space after |for| and |if|.

::: js/src/xpconnect/wrappers/ContentLog.cpp
@@ +280,5 @@
> +
> +  JSAutoByteString propStr;
> +  const char *propName =
> +    propertyID && JSID_IS_STRING(*propertyID) ?
> +    js_AtomToPrintableString(cx, JSID_TO_ATOM(*propertyID), &propStr) :

I don't think you want to use an atom here. Instead, you can simply do: 

JSString *str = propertyID && JSID_IS_STRING(*propertyID) ? JSID_TO_STRING(*propertyID) : nsnull;
const char *propName = str ? propStr.encode(cx, str) : nsnull;

Comment 15

[Drew Willcoxon :adw]

Attachment: mochitest-browser-chrome crash stack

With my logging turned on, mochitest-browser-chrome crashes half-way through.  The stack is attached.  It's a failing assertion:

#2  0x025ed439 in JS_Assert (s=0x2ca990c "shape.methodObject() == prev.toObject()", file=0x2ca92ec "/home/adw/mozilla-central/js/src/jsscope.cpp", ln=1234)

Seems bad, like the instrumentation is changing some JS-related state:  the stack has JIT calls on it, and my ContentLog.cpp is not on the stack at all.

I've only debugged it to this line in my patch:

  char *stackBuf = xpc->DebugPrintJSStack(PR_FALSE, PR_FALSE, PR_FALSE);

If I don't call DebugPrintJSStack, no crash.

Comment 16

[Drew Willcoxon :adw]

I've isolated the crash to the tests in browser/base/content/test/tabview/.  Here's the JS stack when it happens: 

(gdb) call DumpJSStack()
0 anonymous(261) ["chrome://browser/content/tabview.js":672]
    this = [object ChromeWindow @ 0x2488c860 (native @ 0x2488be38)]
1 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>

Unfortunately tabview.js uses a bunch of #includes, so I'll have to track down that line by hand.

Comment 17

[Jason Orendorff [:jorendorff]]

You are right to be surprised that DebugPrintJSStack can cause later problems. Debugging functions should generally be read-only, right?

Well, this one has a side effect, one that is normally innocuous, but in your case it happens to trigger an unrelated bug.

XPConnect::DebugPrintJSStack calls xpc_PrintJSStack
which calls FormatJSStackDump
which calls FormatJSFrame for each JS stack frame
which calls JS_GetFrameScopeChain
which causes the frame's scope chain to be lazily initialized.

This is correct behavior, but it seems to be triggering bug 561359.

Comment 18

[Drew Willcoxon :adw]

This is the tabview.js line where I crash:

  if (typeof options.complete == "function")

http://hg.mozilla.org/mozilla-central/file/e00f1b194440/browser/base/content/tabview/iq.js#l626

Checking the crash stack, it looks like the "complete" property lookup may be the problem.

(In reply to comment #17)
> This is correct behavior, but it seems to be triggering bug 561359.

Thanks for that info, Jason.  Is there anything I can do about it?  Any debugging I can do for you?

Comment 19

[Jason Orendorff [:jorendorff]]

Not unless you can find the memory leak in that patch. See bug 561359 comment 44.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I cloned http://hg.mozilla.org/mozilla-central applied the 'for-review' patch and when building I am experiencing following error:

Creating library xul.lib and object xul.exp
ContentLog.obj : error LNK2019: unresolved external symbol "char const * __cdecl
 js_AtomToPrintableString(struct JSContext *,class JSAtom *,class JSAutoByteStri
ng *)" (?js_AtomToPrintableString@@YAPBDPAUJSContext@@PAVJSAtom@@PAVJSAutoByteSt
ring@@@Z) referenced in function "unsigned int __cdecl LogContentAccessImpl(char
 const *,struct JSContext *,struct JSObject *,struct jsid *)" (?LogContentAccess
Impl@@YAIPBDPAUJSContext@@PAUJSObject@@PAUjsid@@@Z)
xul.dll : fatal error LNK1120: 1 unresolved externals


Here is my .mozconfig

# Options for client.mk.
mk_add_options MOZ_CO_PROJECT=browser
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/_obj-browser-debug

# Options for 'configure' (same as command-line options).
ac_add_options --enable-extensions=default
ac_add_options --enable-application=browser
ac_add_options --enable-debug
ac_add_options --disable-optimize
ac_add_options --enable-shared
ac_add_options --disable-tests
ac_add_options --enable-logging
ac_cv_visibility_pragma=no
ac_add_options --disable-installer
ac_add_options --enable-chrome-format=flat
ac_add_options --enable-debug-symbols=yes
ac_add_options --disable-embedding-tests
ac_add_options --disable-mochitest
ac_add_options --disable-accessibility
ac_add_options --disable-updater
ac_add_options --disable-static
ac_add_options --enable-libxul
ac_add_options --disable-ipc
ac_add_options --disable-angle

Any tips?

Honza

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Btw. I am on Windows Vista.
Honza

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Honza, make the change suggested in the second part of comment 14 to get it to compile

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Felipe Gomes (:felipe) from comment #22)
> Honza, make the change suggested in the second part of comment 14 to get it
> to compile
Yep, it fixed the problem, thanks Felipe!

One more question.

My goal is to use the logging-patch and identify all places in Firebug where chrome touches the content. Such analysis will help me to prepare a plan for Firebug transition to e10s.

As expected, I am getting huge log file. Start Firefox, reload the home page produces: 4MB, 60K+ lines

I would like to redirect the logging into Firebug Tracing Console that we also use to see all logs coming from Firebug itself (often the only way how to debug a debugger).

Would it be possible to expose a simple XPCOM (service) that I could access in JS, register a listener into it and handle all logs dynamically?

The listener could get following parameters:

function contentAccessListener(accessDesc, jscontext, object, property, stack)
{
}

Honza

Comment 24

[Drew Willcoxon :adw]

Attachment: updated patch

This addresses Blake's comments except for these:

(In reply to Blake Kaplan (:mrbkap) from comment #13)
> Instead of looking up the property on the object, you can take the JS object
> and call nsIXPConnect::GetNativeOfWrapper. If that QIs to
> nsIFrameMessageManager, then you have the right JS object.

That didn't work, so this patch still uses JS_GetContextPrivate().

(In reply to Blake Kaplan (:mrbkap) from comment #13)
> There's a tricky one: if (!wrapperObj || !xrayHolder) return wrapperObj;
> might be a success condition if we're not creating an Xray wrapper.

If !wrapperObj, I don't know what to log.  I tried |obj| but that doesn't seem to be right.  JSWrapper::obj_toString() takes a JSObject *wrapper.  So I just left the unconsolidated log calls that Blake mentions in comment 6 (the ones in WrapperFactory::Rewrap and WrapperFactory::WaiveXrayAndWrap).

---

What's left to do:

Wrappers are logged when they're created (and when their properties are accessed), but not if they're later cached and then passed around.  (Caching happens internally to the wrapper implementations, and various front-end JS also holds on to references.)  So for example, when browser.xml's get_docShell() is called every time after the first time, nothing's logged.  (I spent a lot of time trying to find wrapper code paths to catch these for the various types of wrappers.  I found some promising paths, but I ran into the next problem...)

Jez found a false positive in bug 674313.  I don't know how to tell whether a given wrapper (xpcwrappednative at least) is truly content-related, e.g., whether a DOM window is a chrome window or a content window.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Drew Willcoxon :adw from comment #24)
> That didn't work, so this patch still uses JS_GetContextPrivate().

Didn't work how? I still think there's a chance that JS_GetContextPrivate will give you the wrong object.

> 
> (In reply to Blake Kaplan (:mrbkap) from comment #13)
> > There's a tricky one: if (!wrapperObj || !xrayHolder) return wrapperObj;
> > might be a success condition if we're not creating an Xray wrapper.
> 
> If !wrapperObj, I don't know what to log.  I tried |obj| but that doesn't
> seem to be right.

Sorry, I was too brief in my comment: if !wrapperObj, then this is actually a failure case and you don't need to log anything. It's slightly tricky but the idea is that if wrapperObj is null, then we want to return null and if xrayHolder is null, we want to return wrapperObj (since we won't be creating an Xray wrapper).

> Jez found a false positive in bug 674313.  I don't know how to tell whether
> a given wrapper (xpcwrappednative at least) is truly content-related, e.g.,
> whether a DOM window is a chrome window or a content window.

There's a few interfaces that chrome windows implement that content windows don't. In particular, chrome windows implement nsIDOMChromeWindow. Does that help at all?

Comment 26

[Drew Willcoxon :adw]

Attachment: DoubleWrap patch

(In reply to Blake Kaplan (:mrbkap) from comment #25)
> (In reply to Drew Willcoxon :adw from comment #24)
> > That didn't work, so this patch still uses JS_GetContextPrivate().
> 
> Didn't work how? I still think there's a chance that JS_GetContextPrivate
> will give you the wrong object.

GetNativeOfWrapper's return value didn't QI to nsIFrameMessageManager for wrappers I knew to be related to frame scripts.  But maybe my code is wrong, or I'm passing the wrong objects to GetNativeOfWrapper:

  // Ignore message manager scripts.
  nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID()));
  NS_ENSURE_STATE(xpc);
  nsCOMPtr<nsISupports> native(xpc->GetNativeOfWrapper(cx, wrapper));
  if (nsCOMPtr<nsIFrameMessageManager>(do_QueryInterface(native)))
    return NS_OK;

> Sorry, I was too brief in my comment: if !wrapperObj, then this is actually
> a failure case and you don't need to log anything. It's slightly tricky but
> the idea is that if wrapperObj is null, then we want to return null and if
> xrayHolder is null, we want to return wrapperObj (since we won't be creating
> an Xray wrapper).

OK... I'm still not sure exactly where in WrapperFactory::Rewrap to stick the log call (and what object to log) in order to avoid the log call in WaiveXrayAndWrap.  I think you're saying it's somewhere above the if (!wrapperObj || !xrayHolder) conditional, but that's as far as I get.

> There's a few interfaces that chrome windows implement that content windows
> don't. In particular, chrome windows implement nsIDOMChromeWindow. Does that
> help at all?

Yeah... I found that pretty much everything goes through WrapperFactory::DoubleWrap, like, all wrapper "gets".  So this new patch logs that function too and attempts to determine which wrappers are content-related.  Is that OK?  It seems to capture what I want, meaning the gets, in addition to creations that other log sites were already capturing...

Comment 27

[Drew Willcoxon :adw]

Accidentally left three unnecessary #includes in xpcwrappednative.cpp.  I'll remove those if the rest of the patch turns out to be OK.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Drew Willcoxon :adw from comment #26)
>   // Ignore message manager scripts.
>   nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID()));
>   NS_ENSURE_STATE(xpc);
>   nsCOMPtr<nsISupports> native(xpc->GetNativeOfWrapper(cx, wrapper));
>   if (nsCOMPtr<nsIFrameMessageManager>(do_QueryInterface(native)))
>     return NS_OK;

Yeah, you actually need to get the global object of the wrapper here (using JS_GetGlobalForObject(cx, wrapper)) before doing the QI. Otherwise this code is basically asking if every wrapper it sees is a message manager itself.

> OK... I'm still not sure exactly where in WrapperFactory::Rewrap to stick
> the log call (and what object to log) in order to avoid the log call in
> WaiveXrayAndWrap.  I think you're saying it's somewhere above the if
> (!wrapperObj || !xrayHolder) conditional, but that's as far as I get.

I think the current code is correct.

> Yeah... I found that pretty much everything goes through
> WrapperFactory::DoubleWrap, like, all wrapper "gets".  So this new patch
> logs that function too and attempts to determine which wrappers are
> content-related.  Is that OK?  It seems to capture what I want, meaning the
> gets, in addition to creations that other log sites were already capturing...

I think that logging in DoubleWrap will double-log for most objects. DoubleWrap is only ever called from WrapperFactory::PrepareForWrapping which is always called directly before WrapperFactory::Rewrap, which is already logged.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) from comment #28)
> Yeah, you actually need to get the global object of the wrapper here (using
> JS_GetGlobalForObject(cx, wrapper)) before doing the QI. Otherwise this code
> is basically asking if every wrapper it sees is a message manager itself.

Oops, sorry, I confused myself here. Instead of using the wrapper or JS_GetGlobalObject(cx) you probably want to use JS_GetGlobalForScopeChain(cx).

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 558953 [details] [diff] [review]
DoubleWrap patch

This looks really good. I just want to iterate one more time on the JS_GetGlobalObject(cx) thing.

Comment 31

[Drew Willcoxon :adw]

JS_GetGlobalForScopeChain didn't work:

  nsCOMPtr<nsISupports> native(
    xpc->GetNativeOfWrapper(cx, JS_GetGlobalForScopeChain(cx)));
  if (nsCOMPtr<nsIFrameMessageManager>(do_QueryInterface(native)))
    return NS_OK;

This didn't catch stacks from extensions-content.js, which I know to be a frame script.  Sorry if I'm being dense and this isn't the right incantation either.

But JS_GetGlobalObject did work.  I didn't try JS_GetGlobalForObject.

FWIW, this is the reason I was using JS_GetContextPrivate:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsInProcessTabChildGlobal.cpp?rev=91aa25a681a5#328

nsInProcessTabChildGlobal implements nsIInProcessContentFrameMessageManager, the specific type of message manager I want to ignore.

(In reply to Blake Kaplan (:mrbkap) from comment #28)
> I think that logging in DoubleWrap will double-log for most objects.
> DoubleWrap is only ever called from WrapperFactory::PrepareForWrapping which
> is always called directly before WrapperFactory::Rewrap, which is already
> logged.

Logging DoubleWrap at the site I do in the patch definitely catches wrapper accesses that I need to catch and that aren't caught by the logging in Rewrap.  I tried broadening the scope of what's logged in Rewrap by checking isObjContentRelated in addition to the shouldLogContentAccess flag that I set, but that still missed wrappers that DoubleWrap catches.

And I noticed I'm not catching the HTML document being passed as an argument to the getBrowserIndexForDocument method of tabbrowser.  I don't know why or where I would put more logging in to catch it.  I'm really tired of this.

Comment 32

[Dietrich Ayala (:dietrich)]

Assigning to Blake to take it from here for the rest of these dark closets of the platform bits, or to delegate as makes sense.

Previous status was that the patch as-is (even a while back) provided enough good information to be usable for detection of many e10s-conversion-needing spots. Is that still the case?

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

I'm not actually working on this and it isn't clear that we need it anymore.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.