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]
:
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]
no flags Details | Diff | Splinter Review
test browser (10.32 KB, patch)
2010-05-06 06:14 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (79.46 KB, patch)
2010-05-06 06:24 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (79.49 KB, patch)
2010-05-06 06:59 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (83.80 KB, patch)
2010-05-10 06:10 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (85.28 KB, patch)
2010-05-10 14:29 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v7 (89.94 KB, patch)
2010-05-11 15:37 PDT, Olli Pettay [:smaug]
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]
no flags Details | Diff | Splinter Review

Description 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-03-02 11:46:25 PST
*** Bug 534254 has been marked as a duplicate of this bug. ***
Comment 3 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-06 06:24:58 PDT
Created attachment 443869 [details] [diff] [review]
patch

This should behave pretty closely like e10s-mm.
Comment 6 Olli Pettay [:smaug] 2010-05-06 06:26:00 PDT
When merging e10s to m-c, some of the code duplication can be removed.
Comment 7 Olli Pettay [:smaug] 2010-05-06 06:27:16 PDT
Doug, would be great if this could be tested with Fennec.
Comment 8 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-06 06:53:01 PDT
updated patch coming..
Comment 10 Olli Pettay [:smaug] 2010-05-06 06:59:28 PDT
Created attachment 443872 [details] [diff] [review]
patch
Comment 11 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-06 08:53:29 PDT
And to clarify, the patch is for m-c, not for e10s branch.
Comment 13 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-10 06:10:41 PDT
Locally chrome tests pass now, but I posted the patch anyway to tryserver.
Comment 19 Olli Pettay [:smaug] 2010-05-10 09:02:10 PDT
Uh, tryserver doesn't like the patch. Investigating.
Comment 20 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-12 01:23:13 PDT
Comment on attachment 444767 [details] [diff] [review]
v7

Tryserver liked this better :)
Comment 25 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 Olli Pettay [:smaug] 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 :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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-18 03:18:34 PDT
Backed out. There is some crash.
http://hg.mozilla.org/mozilla-central/rev/5127e4bfa127
Comment 30 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 2010-05-18 07:51:13 PDT
Only known oranges. Marking fixed.
Please open new bugs for issues and feature requests.
Comment 32 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 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 Olli Pettay [:smaug] 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 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 Olli Pettay [:smaug] 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 Olli Pettay [:smaug] 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 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 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 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 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.