e10s: Javascript errors should be proxied to chrome

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

(Whiteboard: [fennec-checkin-postb1])

Attachments

(1 attachment, 5 obsolete attachments)

The silent failures I keep getting are starting to make me very sad.  Digging through debug console output is not a fun process.
Blocks: e10s
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.
Assignee: nobody → josh
Depends on: 528250
Note: patch depends on the build tier unification in bug 528250.
This is better, and actually gets the job done.  I think I can one up it, though.
Attachment #429369 - Attachment is obsolete: true
Unlike previous patches, messages now remote themselves.  The system appears to work flawlessly.
Attachment #429383 - Attachment is obsolete: true
Attachment #429386 - Flags: review?(benjamin)
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.
Attachment #429386 - Flags: review?(benjamin) → review?(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?
No, IPDL methods have to be main-thread only.
Another option is to add a console listener in the child process which sends stuff over to the parent process...
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...
Blocks: 556846
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.
Posted patch Patch (obsolete) — Splinter Review
This is effectively the same patch as previously, but now doesn't break when messages are logged from arbitrary threads.
Attachment #429386 - Attachment is obsolete: true
Attachment #442909 - Flags: review?(bzbarsky)
Attachment #429386 - Flags: review?(bzbarsky)
+++ b/dom/ipc/Makefile.in
 		-I$(topsrcdir)/chrome/src \
+		-I$(srcdir)/../../xpcom/base \

I'd say

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

makes more sense.
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...
tracking-fennec: --- → 2.0+
Comment on attachment 442909 [details] [diff] [review]
Patch

New patch coming up.
Attachment #442909 - Attachment is obsolete: true
Attachment #442909 - Flags: review?(bzbarsky)
Attachment #477531 - Flags: review?(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
Attachment #477531 - Flags: review?(bzbarsky) → review+
Attachment #477531 - Attachment is obsolete: true
Incorrect string usage addressed.
Whiteboard: [fennec-checkin-postb1]
tracking-fennec: 2.0+ → 2.0b2+
pushed http://hg.mozilla.org/mozilla-central/rev/5f0370936fc9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.