Last Comment Bug 549682 - Port the message-manager API to mozilla-central
: Port the message-manager API to mozilla-central
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
-- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (pto-ish for couple of days)
:
: Andrew Overholt [:overholt]
Mentors:
: 534254 (view as bug list)
Depends on: 567673 582441 542242 574049 578960
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-02 11:41 PST by Benjamin Smedberg [:bsmedberg]
Modified: 2010-11-19 13:20 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip, misses still all the JS handling etc, so basically perhaps 3/5 done. (61.16 KB, patch)
2010-05-05 15:42 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
test browser (10.32 KB, patch)
2010-05-06 06:14 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
patch (79.46 KB, patch)
2010-05-06 06:24 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
patch (79.49 KB, patch)
2010-05-06 06:59 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
patch (83.80 KB, patch)
2010-05-10 06:10 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
patch (85.28 KB, patch)
2010-05-10 14:29 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v7 (89.94 KB, patch)
2010-05-11 15:37 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
jst: review+
Details | Diff | Splinter Review
keep MM alive while calling loadFrameScript (89.98 KB, patch)
2010-05-18 05:29 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review

Description User image Benjamin Smedberg [:bsmedberg] 2010-03-02 11:41:39 PST
As soon as the message-manager API is reasonably stable-seeming, we should port it to mozilla-central so that code can start transitioning to it without having e10s ifdefs.
Comment 1 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-03-02 11:46:09 PST
This is a dup of bug 534254, but I'll mark that to be a dup of this.

And yes, I plan to do this once the API is reasonable stable.
Comment 2 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-03-02 11:46:25 PST
*** Bug 534254 has been marked as a duplicate of this bug. ***
Comment 3 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-05 15:42:16 PDT
Created attachment 443742 [details] [diff] [review]
wip, misses still all the JS handling etc, so basically perhaps 3/5 done.
Comment 4 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:14:04 PDT
Created attachment 443867 [details] [diff] [review]
test browser

This has basically all the same tests as for e10s mm, but without CPOW
(which is something in-process-mm won't support, since CPOWs will be removed
from e10s too)
Comment 5 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:24:58 PDT
Created attachment 443869 [details] [diff] [review]
patch

This should behave pretty closely like e10s-mm.
Comment 6 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:26:00 PDT
When merging e10s to m-c, some of the code duplication can be removed.
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:27:16 PDT
Doug, would be great if this could be tested with Fennec.
Comment 8 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:37:51 PDT
Seem like the patch may not apply quite clean with trunk, at least not without
some --fuzz
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:53:01 PDT
updated patch coming..
Comment 10 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 06:59:28 PDT
Created attachment 443872 [details] [diff] [review]
patch
Comment 11 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 08:34:17 PDT
One thing I may need to change, and which would work differently than in
E10s is script loading.
In E10s it happens asynchronously, but I think in non-E10s it should
happen sync so that scripts are ready when the page is being loaded.

The change would be trivial; just don't use nsAsyncScriptLoader,
but call LoadRemoteScript in LoadScript.
Comment 12 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 08:53:29 PDT
And to clarify, the patch is for m-c, not for e10s branch.
Comment 13 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 09:34:04 PDT
Hmm, tryserver doesn't seem to like the patch.
I need to look at the failures.
But I think it is still ok to review the patch.
Comment 14 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-06 09:35:37 PDT
(In reply to comment #13)
> Hmm, tryserver doesn't seem to like the patch.
...which is not very surprising. Changing how events are propagated is
rather regression risky.
Comment 15 User image Benjamin Stover (:stechz) 2010-05-06 13:46:38 PDT
Tried this patch out with e10s Fennec, and this gets us a long way towards the goal of working with remote and non-remote browsers.  Great work!
Comment 16 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-07 04:09:36 PDT
Thanks for testing. I'll try to fix the tryserver failures today/tomorrow and
I'll probably change the script loading to happen synchronously.
Comment 17 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-10 06:10:18 PDT
Created attachment 444389 [details] [diff] [review]
patch

The previous patch was missing the case where frameloaders are swapped.
I think that is a case which isn't handled at all in E10s atm.
Comment 18 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-10 06:10:41 PDT
Locally chrome tests pass now, but I posted the patch anyway to tryserver.
Comment 19 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-10 09:02:10 PDT
Uh, tryserver doesn't like the patch. Investigating.
Comment 20 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-10 14:29:06 PDT
Created attachment 444478 [details] [diff] [review]
patch

Yet another patch.  Crossing fingers, maybe tryserver likes this.
At least testing this locally looks promising.
The previous patch was actually unlinking too much in some cases.
Comment 21 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-11 01:39:49 PDT
The latest patch seems to have passed tests on tryserver, though
some random looking orange in some builds.
Comment 22 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-11 12:51:38 PDT
Comment on attachment 444478 [details] [diff] [review]
patch

Bah, I found still something to fix. And chrome tests are coming too.
Comment 23 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-11 15:37:45 PDT
Created attachment 444767 [details] [diff] [review]
v7

JSPrincipal handling is extremely ugly, and not documented at all.
This patch doesn't leak principals anymore. I'll fix E10s in bug 565175
(if the problem is also in E10s).
Comment 24 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-12 01:23:13 PDT
Comment on attachment 444767 [details] [diff] [review]
v7

Tryserver liked this better :)
Comment 25 User image Johnny Stenback (:jst, jst@mozilla.com) 2010-05-12 16:20:42 PDT
Comment on attachment 444767 [details] [diff] [review]
v7

Looks good.
Comment 26 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-18 02:05:26 PDT
Landed http://hg.mozilla.org/mozilla-central/rev/b62ac40565f2
But let's see if that works before marking this fixed.
Comment 27 User image :Ms2ger (⌚ UTC+1/+2) 2010-05-18 02:51:04 PDT
In nsContentUtils.cpp:

    7.38 -  if (!aDoc->GetWindow()->GetChromeEventHandler())
    7.39 +
    7.40 +  nsPIDOMEventTarget* piTarget = aDoc->GetWindow()->GetChromeEventHandler();
    7.41 +  if (!target)
    7.42      return NS_ERROR_INVALID_ARG;

Shouldn't this check for piTarget instead?
Comment 28 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-18 02:59:04 PDT
Indeed. Thanks!

I'll fix in a followup hg push, if the patch keeps tbox green.
Comment 29 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-18 03:18:34 PDT
Backed out. There is some crash.
http://hg.mozilla.org/mozilla-central/rev/5127e4bfa127
Comment 30 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-18 05:29:39 PDT
Created attachment 445939 [details] [diff] [review]
keep MM alive while calling loadFrameScript

This fixes also the piTarget thing.

I'll push this. At least locally I can't reproduce the problem tbox showed.
Comment 31 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-18 07:51:13 PDT
Only known oranges. Marking fixed.
Please open new bugs for issues and feature requests.
Comment 32 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-22 13:14:25 PDT
smaug, these changes make for a tricky merge of m-c to e10s.  Do you mind doing it?
Comment 33 User image Robert Sayre 2010-05-22 13:57:11 PDT
I was looking at this checkin while merging TM, and it looks there are a lot of JSAPI calls with unchecked return values. What's up with that?
Comment 34 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-23 13:03:38 PDT
What *a lot calls* with unchecked return values?
It is ok, IMO,  if JS_DefineProperty fails in this case, and in other cases the
return value is checked.
There is also no need to check the return value of JS_EvaluateUCScriptForPrincipals, as far as I see.


And about merging, yes, I guess I need to do that.
Before starting to write the patch I was arguing that it should be written
for e10s, not m-c, but others didn't agree.
Comment 35 User image Brendan Eich [:brendan] 2010-05-23 14:01:44 PDT
(In reply to comment #34)
> What *a lot calls* with unchecked return values?
> It is ok, IMO,  if JS_DefineProperty fails in this case, and in other cases the
> return value is checked.
> There is also no need to check the return value of
> JS_EvaluateUCScriptForPrincipals, as far as I see.

It's not ok to fail to report an uncaught exception. Is this API call guaranteed to be happening when cx has no active frames on it (!JS_IsActive(cx))?

If so, ok; the engine will report uncaught exceptions as this API unwinds (there is a JS context option, JSOPTION_DONT_REPORT_UNCAUGHT, that you can set to avoid this default behavior).

If not, then you are adding hard-to-find bugs by suppressing failure (exception, error) instead of propagating it.

/be
Comment 36 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-23 14:11:07 PDT
MDC documentation doesn't say anything about handling uncaught exceptions.
It says just "On success, JS_DefineProperty returns JS_TRUE. If the property already exists or cannot be created, JS_DefineProperty returns JS_FALSE."

And for JS_EvaluateUCScriptForPrincipals, the code comes from e10s content
process, where there isn't any place to report any exceptions (currently).
And MDC says just "If the script compiles and executes successfully, *rval receives the value from the last-executed expression statement processed in the script, and JS_EvaluateScriptForPrincipals or JS_EvaluateUCScriptForPrincipals returns JS_TRUE. Otherwise it returns JS_FALSE, and the value left in *rval is undefined. "
Nothing about exceptions. Based on that how could API user know that
one should handle the return value?


(jsapi.h is even more scarce than MDC)
Comment 37 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-05-23 14:18:31 PDT
And how does the exception propagate from JS_EvaluateUCScriptForPrincipals?
Does it propagate to caller? (Which should be JS). That would be
pretty reasonable behavior for loadFrameScript().
Comment 38 User image Brendan Eich [:brendan] 2010-05-23 15:02:14 PDT
jsapi.h docs are on MDC, not in the .h file itself; see specifically

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_User_Guide#Errors_and_exceptions

See also

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Phrasebook

jst and mrbkap know the drill; so do others.

/be
Comment 39 User image Robert Sayre 2010-05-23 15:29:50 PDT
This is what I spotted:

179       JS_Stringify(ctx, &v, nsnull, JSVAL_NULL, JSONCreator, &aJSON);
(unchecked)

213       for (PRUint32 i = 0; i < len; ++i) {
214         jsval ret = JSVAL_VOID;
215         nsAutoGCRoot root(&ret, &rv);
216         NS_ENSURE_SUCCESS(rv, rv);
217         JSONParser* parser = JS_BeginJSONParse(ctx, &ret);
(needs null check)

218         JSBool ok = JS_ConsumeJSONText(ctx, parser, (jschar*)retval[i].get(),
219                                        (uint32)retval[i].Length());
220         ok = JS_FinishJSONParse(ctx, parser, JSVAL_NULL) && ok;
221         if (ok) {
222           dest[i] = ret;
223         }
(loop continues after failure)

224       }


403         JS_CallFunctionValue(mContext, thisObject,
404                              funval, 1, argv, &rval);
(unchecked)

408               JS_Stringify(mContext, &rval, nsnull, JSVAL_NULL,
409                            JSONCreator, &json)) {
(unchecked)
Comment 40 User image Brian Crowder 2010-06-17 17:53:45 PDT
+    nsCOMPtr<nsIContentFrameMessageManager> mm = mChildManagers[i];

This needs to become

+    nsCOMPtr<nsIContentFrameMessageManager> mm = do_QueryInterface(mChildManagers[i]);

probably due to a recent change in e10s?  ....  want a new bug for this?
Comment 41 User image Nickolay_Ponomarev 2010-08-18 13:34:12 PDT
dev-doc-needed: https://wiki.mozilla.org/Content_Process_Event_Handlers

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