548904 - e10s: Javascript errors should be proxied to chrome

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

The silent failures I keep getting are starting to make me very sad.  Digging through debug console output is not a fun process.

Comment 1

[Josh Matthews [:jdm]]

Attachment: WIP basic remoting of nsConsoleService::LogMessage

This just does a straight proxy of LogMessage, so everything shows up in the parent as a generic console message.  This is less than ideal, but gets the job done for the time being.

Comment 2

[Josh Matthews [:jdm]]

Note: patch depends on the build tier unification in bug 528250.

Comment 3

[Josh Matthews [:jdm]]

Attachment: WIP proper remoting of script errors

This is better, and actually gets the job done.  I think I can one up it, though.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Delegate message remoting to each message type

Unlike previous patches, messages now remote themselves.  The system appears to work flawlessly.

Comment 5

[Josh Matthews [:jdm]]

Hmm, there's something fishy happening on shutdown where the child process aborts when trying to send a script error.  I'm looking into that; I would still appreciate a review of the current patch because I believe the method is sound.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  So a few questions:

1)  Wouldn't it make more sense to do the sending in the console service instead?
2)  LogMessage can happen on an arbitrary thread.  Can the SendRemoteMessage
    stuff run on an arbitrary thread?

Comment 7

[Benjamin Smedberg]

No, IPDL methods have to be main-thread only.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Another option is to add a console listener in the child process which sends stuff over to the parent process...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Just in terms of where to put this code, that is.  The main drawback is it would have to know about the kinds of console messages that can happen...

Comment 10

[Josh Matthews [:jdm]]

I've got a new version which dispatches an event to the main thread to trigger the IPDL message.  In my mind, forcing message types to be aware of how to remote themselves is cleaner than an all-knowing listener or service.  It also reduces  boilerplate significantly for things like nsScriptError (ie. rv = message->GetX(&x); NS_ENSURE_SUCCESS(rv, rv)).  However, if you'd prefer the omnipotent version, I can whip that up as well.

Comment 11

[Josh Matthews [:jdm]]

Attachment: Patch

This is effectively the same patch as previously, but now doesn't break when messages are logged from arbitrary threads.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

+++ b/dom/ipc/Makefile.in
 		-I$(topsrcdir)/chrome/src \
+		-I$(srcdir)/../../xpcom/base \

I'd say

+		-I$(topsrcdir)/xpcom/base \

makes more sense.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So with this patch, console listeners in the content process see no messages.  Is that desired?

If not, it seems cleaner to allow listeners but have a listener that calls SendRemoteMessage.  Listeners _are_ notified on the main thread (the console service handles this), and that should make things Just Work.  Might even be less boilerplate...

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 442909 [details] [diff] [review]
Patch

New patch coming up.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Remote console messages from content to chrome via a listener.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 477531 [details] [diff] [review]
Remote console messages from content to chrome via a listener.

>+++ b/dom/ipc/ContentChild.cpp
>+        nsAutoPtr<char> category;
>+        char *tmp;

No, this is totally broken.  This will deallocate with delete[] memory that was allocated with some other allocator.

The right way to do this is:

  nsXPIDLCString category;
  .....
  rv = scriptError->GetCategory(getter_Copies(category));


r=me with that fixed

Comment 17

[Josh Matthews [:jdm]]

Attachment: Remote console messages from content to chrome via a listener.

Incorrect string usage addressed.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/5f0370936fc9