937317 - Implement HTML5 Script Settings Object Stack

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Just need to do this.

Comment 1

[Bobby Holley (:bholley)]

I was going to write this as a continuation of the private thread "JS_DescribeScriptedCaller and selfHosted" from last summer, but I think it's better to move to a bug instead.

Hixie has now refactored the spec to describe a 'script settings object', which has a 1:1 mapping with globals. So we should probably define a corresponding object, and support it for both worker and non-worker globals.

In the new spec model, there's a stack of incumbent scripts, some of which are also marked 'candidate entry scripts'. The 'candidate entry script' thing only happens during event dispatch as far as I can tell.

The common case is 'When a JavaScript SourceElements production is to be evaluated', which causes us to push a global onto the stack (and generally not label it as a script entry point). WebIDL callbacks capture the incumbent script at the time when they were passed, and restore that onto the stack when they are invoked.

Obviously, we don't want an explicit representation of the entire stack of incumbent scripts, because that duplicates information already inside the JS engine, and would require munging the stack at every function call. So it seems like we want to do something like the following:

* Define some kind of script settings type. This could either be a separate object that both Window and WorkerGlobalScope create, or it could be an interface that both native globals QI to. I think the latter makes more sense unless there's something I haven't thought of. Let's call this nsIScriptSettingsObject for now.
* Add a per-CycleCollectedJSRuntime stack of ScriptSettingsStackEntries. Each entry has, at minimum, an nsIScriptSettingsObject and a bool indicating whether or not it's a candidate entry script.
* Add a hook in the JS engine such that, whenever we push one of these entries onto the stack, we increment a counter on the topmost function activation. If this is nonzero, JS_HasScriptedCallerOverride returns true, and JS_DescribeScriptedCaller asserts. We should assert that the entry is zero when the activation dies.
* Build a Gecko-side wrapper around this junk so that callers can just invoke GetIncumbentScript and GetEntryScript, and it will return the proper nsIScriptSettingsObject from a combination of the data in the JS engine and the data in the ScriptSettingsStack.
* When entering a top-level script for a page, push its global onto the stack, and mark it as a candidate entry script.
* Capture the result of GetIncumbentScript when passing a callback to WebIDL, and store the result on the callback.
* When invoking a WebIDL callback, push the stored nsIScriptSettingsObject. If we're doing event dispatch, tag it as a candidate entry point. Otherwise, don't.

There's also the question of what to do when we spin the event loop. The spec currently says that the stack is set aside. As I described in bug 924905 comment 28, I think we should generally get this changed, and move to a model whereby you can see whatever parts of the stack you subsume. But we could introduce some kind of null pushing for now.

Thoughts? Did I miss anything?

Comment 2

[Bobby Holley (:bholley)]

Oh, and as for the naming of the RAII classes:

We could do AutoEntryScript and AutoIncumbentScript. But it might make more sense to do:

AutoScriptSettings foo(nsIScriptSettings arg);
foo.setCandidateEntryScript();

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't understand the candidate entry script bit, and I doubt it's web-compatible ascurrently written.  I think every call of a WebIDL callback should be an entry script.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or more precisely, an entry script settings object, since there may be no "script" there at all.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So for now I've created AutoEntryScript and AutoIncumbentScript.  They both take an nsIGlobalObject.  The former also takes (for now?) a JSContext and a boolean, because it turns out that there are situations in which we do not in fact have an nsIGlobalObject and getting from an nsIGlobalObject to an nsJSContext in the cases when we _do_ have the former is slow (QI, really?).

Once we nix the JSContext dependence we'll be able to clean that up....

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bobby, somethinkg like this what you were thinking of?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I could also try to push more of the exception-reporting in there if we want, but that's the main thing that could go in there.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 832460 [details] [diff] [review]
Bobby, somethinkg like this what you were thinking of?

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

I think that Auto{Entry,Incumbent}Script.{cpp,h} should be merged into a ScriptSettings.{cpp,h}. I'll stick the other relevant stuff in that file as well.

r=bholley with comments addressed. Since inbound is going to be closed for a while, maybe you can just upload the new patch and I'll manually apply it for now?

::: dom/bindings/CallbackObject.cpp
@@ +107,2 @@
>    } else {
>      cx = workers::GetCurrentThreadJSContext();

We need to set globalObject here as well, right?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I think that Auto{Entry,Incumbent}Script.{cpp,h} should be merged into a ScriptSettings.

OK.

> maybe you can just upload the new patch 

Sure thing.

> We need to set globalObject here as well, right?

It's already null.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Or more to the point, I had no idea how I go about getting an nsIGlobalObject on a worker and the script settings stuff clearly has to handle a null nsIGlobalObject, since that's what we get in the safe context case.  I think I can see how to get an nsIGlobalObject on a worker, though, so maybe I should do that...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: With comments addressed

Comment 12

[Bobby Holley (:bholley)]

I think it makes more sense to land this all in one go in bug 938878.

Comment 13

[Bobby Holley (:bholley)]

Or actually, no. This bug has more context. I'm just going to rename it. Sorry for the churn.

Comment 15

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2007f8c299ff

Comment 16

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=457de8778da0

Comment 17

[Bobby Holley (:bholley)]

I'm still working on the tests, but so far everything appears to be working. Doing a try push in case Bob wants to do anything over the next few days (I won't be doing much work).

https://tbpl.mozilla.org/?tree=Try&rev=49cf546423bf

Alternatively, you can pull them from github here:

https://github.com/bholley/gecko-dev/tree/entryscript

Comment 18

[Bobby Holley (:bholley)]

Try push was green, and I've got tests that pass. Uploading and flagging for review.

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make TabChildGlobal implement nsIGlobalObject. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Factor out the shareable parts of CallSetup into AutoEntryScript and AutoIncubentScript. v3

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement basic script settings stack machinery. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 4 - Manipulate the script settings stack from the RAII classes. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement and expose GetIncumbentGlobal. v1

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 6 - Pass the entire CallbackObject to CallSetup. v1

We need this so that we can grab the incumbent global that we stashed on the
CallbackObject.

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 7 - When invoking a callback object, restore the incumbent script settings object from when the callback was created. v1

See the 'incumbent script' stuff in the WebIDL spec.

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 8 - Replace all instance of null cx pushing with AutoSystemCaller. v1

This is an easy bonus chunk of the work to phase out cx pushing in the browser.

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 9 - Tests. v1

Comment 28

[Bobby Holley (:bholley)]

Attachment: rollup.patch

rollup patch for reference

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342034 [details] [diff] [review]
Part 1 - Make TabChildGlobal implement nsIGlobalObject. v1

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342035 [details] [diff] [review]
Part 2 - Factor out the shareable parts of CallSetup into AutoEntryScript and AutoIncubentScript. v3

>+    MOZ_ASSERT(aIsMainThread, "cx is mandatory off-main-thread");

This should be documented in the header, I think.

Is there an existing bug about making GetNativeForGlobal() work with WebIDL globals?  If not, please file one.

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342035 [details] [diff] [review]
Part 2 - Factor out the shareable parts of CallSetup into AutoEntryScript and AutoIncubentScript. v3

Though note that AutoIncumbentScript seems unused so far.  That's OK, since it's a no-op, but is that expected here?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342036 [details] [diff] [review]
Part 3 - Implement basic script settings stack machinery. v1

>+  void Push(ScriptSettingsStackEntry& aSettings) {

If we store the pointer here, I think the caller should pass in a pointer too.  Otherwise it's too easy to accidentally pass a reference to something on the stack or some such and then unwind the stack (whereas taking a pointer-to-stack immediately makes people check lifetimes).

>+void InitScriptSettings();

Document that this only needs to be called once, during startup?  And maybe fix the docs for ThreadLocal to make it clear that it only needs to be inited once, not once per thread or something?

I assume there's a reason to use vector instead of nsTArray?

I still don't understand when a script setting stack entry would _not_ be an entry point candidate.  Can you please point me to an explanation for that?

r=me modulo all that

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342036 [details] [diff] [review]
Part 3 - Implement basic script settings stack machinery. v1

Oh, I see, the non-entry things just track incumbent scripts.  Gotcha.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342037 [details] [diff] [review]
Part 4 - Manipulate the script settings stack from the RAII classes. v1

>+  mStack.Push(ScriptSettingsStackEntry::SystemSingleton);

PushSystem?

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342038 [details] [diff] [review]
Part 5 - Implement and expose GetIncumbentGlobal. v1

>+  JSContext *cx = nsContentUtils::GetCurrentJSContext();

That's mainthread-only, no?  Why is that ok?  I don't think it is.

>+  , mOverride(nsContentUtils::GetCurrentJSContext())

And here.

The JS parts will need review by a JS peer, obviously.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342040 [details] [diff] [review]
Part 6 - Pass the entire CallbackObject to CallSetup. v1

r=me

Though those (preexisting) comments about JSAutoRequest worry me: what happens on workers?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342041 [details] [diff] [review]
Part 7 - When invoking a callback object, restore the incumbent script settings object from when the callback was created. v1

Ah, this fixes the worker GetIncumbentGlobal issue, ok.  That should be in the earlier relevant patch.

> nsDOMEventTargetHelper::SetEventHandler(nsIAtom* aType,

This should presumably use the incumbent global, not null?

>+++ b/content/events/src/nsEventListenerManager.cpp

This should probably use whatever global we just used to compile?  Though maybe it doesn't matter, since that'll be the incumbent global of the function anyway.

>+++ b/dom/base/nsGlobalWindow.cpp
>+      handler = new EventHandlerNonNull(callable, nullptr);                  \

Again, why not the incumbent global here?

>+      handler = new OnErrorEventHandlerNonNull(callable, nullptr);           \

And here.

>+      handler = new OnBeforeUnloadEventHandlerNonNull(callable, nullptr);    \

And here.

>+++ b/dom/bindings/CallbackObject.cpp
>+  tmp->mIncumbentGlobal = nullptr;

  NS_IMPL_CYCLE_COLLECTION_UNLINK(mIncumbentGlobal)

>+    // XXXbholley - Is this right? What are these used for?

Ideally, nothing.  The only in-tree consumers are NodeIterator/TreeWalker, and they would only use if if you somehow create one with an XPCOM callback (that wraps a JS object!) and then get .filter from JS.  Maybe we should just make this thing always return null if it's not already a webidl callback and not worry about this situation.

r=me modulo those nits

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342043 [details] [diff] [review]
Part 8 - Replace all instance of null cx pushing with AutoSystemCaller. v1

>+++ b/content/base/src/nsImageLoadingContent.cpp

Actually, I think the bits here can just go away.  They should have in bug 880340; I'm sorry I didn't notice that they were left behind.

>+++ b/dom/src/geolocation/nsGeolocation.cpp

I strongly suspect the bits here are also not needed now that we're using WebIDL callbacks (which don't just reuse whatever JSContext is on the stack).  Can you please double-check that?

>+++ b/layout/base/nsLayoutUtils.cpp

This one can go away just like the ones in nsImageLoadingContent.

>+++ b/layout/generic/nsVideoFrame.cpp

And likewise here.

r=me with those fixed.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342044 [details] [diff] [review]
Part 9 - Tests. v1

Maybe use DOM promises instead of the semi=deprecated Promise.jsm?

You probably want a test for an event handler compiled from a content attribute.

r=me

Comment 40

[Boris Zbarsky [:bzbarsky]]

And also, thank you for doing this!

Comment 41

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #30)
> Is there an existing bug about making GetNativeForGlobal() work with WebIDL
> globals?  If not, please file one.

filed bug 946289.

(In reply to Boris Zbarsky [:bz] from comment #32)

> I assume there's a reason to use vector instead of nsTArray?

I think at some point I chose it because of the copying semantics or something, but I can't seem to recall why. Given that the current design just uses opaque unowned pointers, nsTArray should be fine. I'll switch to that.

(In reply to Boris Zbarsky [:bz] from comment #36)
> Though those (preexisting) comments about JSAutoRequest worry me: what
> happens on workers?

I think there's an explicit JSAutoRequest that gets constructed somewhere up high in the worker event loop. If there weren't, we'd assert all over the place.

(In reply to Boris Zbarsky [:bz] from comment #37)
> Ah, this fixes the worker GetIncumbentGlobal issue, ok.  That should be in
> the earlier relevant patch.

Yep. Just a mismerge on my part. I'll fix it.

Comment 42

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement and expose GetIncumbentGlobal. v2

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342467 [details] [diff] [review]
Part 5 - Implement and expose GetIncumbentGlobal. v2

>+  , mOverride(nsContentUtils::GetCurrentJSContext())

This still needs to be fixed.

Can we just make nsContentUtils::GetCurrentJSContext do the IsMainThread check?  Or add a new function that does the right thing and call it both places we need this in this patch?

Comment 44

[Bobby Holley (:bholley)]

Attachment: Part 7.5 - Return null from ToWebIDLCallback if one doesn't already exist. v1

Comment 45

[Bobby Holley (:bholley)]

Attachment: Part 8.5 - Remove unnecessary AutoSystemCaller usage. v1 r=bz

See bug 937317 comment 38.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342501 [details] [diff] [review]
Part 7.5 - Return null from ToWebIDLCallback if one doesn't already exist. v1

r=me

Comment 47

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement and expose GetIncumbentGlobal. v3

Sorry for missing that. Putting this in nsContentUtils is an obvious
improvement.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342509 [details] [diff] [review]
Part 5 - Implement and expose GetIncumbentGlobal. v3

Yes, much more like it.

You still want the review from Luke.

Comment 49

[Luke Wagner [:luke]]

Comment on attachment 8342509 [details] [diff] [review]
Part 5 - Implement and expose GetIncumbentGlobal. v3

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

Nice to see all this coming together!  Is there a good high-level summary of what the incumbent global rules are?  I know we had an email thread where we agreed on something a couple months ago, but I forget exactly what we agreed and I don't know if the rules changed since then :)

Nit: "ScriptedCallerOverride" sounds a bit more intimidating than it really is.  IIUC, all we're doing is hiding the current activation, so could the phrase used in all the identifiers be "HideScriptedCaller"?  As in:
  JS_HideScriptedCaller
  JS_UnhideScriptedCaller
  JSAutoHideScriptedCaller
  Activation::hideScriptedCallerCount_;
  Activation::hideScriptedCaller()
  Activation::unhideScriptedCaller()
  Activation::scriptedCallerIsHidden()
?  Comments could be updated to use this terminology as well.  I'd also specifically mention what happens when you've hid the top activation and then, before unhiding it, you reenter the VM (which pushes a new unhidden activation, iiuc).

Also, can you put the new jsapi bits in namespace JS?

::: js/src/jsapi.cpp
@@ +6110,5 @@
>  
> +    // If there's an override, the embedding wants us to return null here so
> +    // that it can check its own stack.
> +    if (i.activation()->hasScriptedCallerOverride())
> +        return false;

For symmetry with the comment below, can you use:

  if (cx->runtime()->mainThread().activation()->hasScriptedCallerOverride())
      return false;

and put it before the NonBuiltinScriptFrameIter decl.

@@ +6129,5 @@
> +    // anything.
> +    NonBuiltinScriptFrameIter i(cx);
> +    if (i.done())
> +        return;
> +    i.activation()->addScriptedCallerOverride();

I think what you want is cx->runtime()->mainThread().activation().

Comment 50

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement and expose GetIncumbentGlobal. v4 r=bz

Addressed luke's review comments.

Comment 51

[Luke Wagner [:luke]]

Comment on attachment 8342738 [details] [diff] [review]
Part 5 - Implement and expose GetIncumbentGlobal. v4 r=bz

Great!

Comment 52

[Bobby Holley (:bholley)]

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=5742c0f93432

Comment 53

[Bobby Holley (:bholley)]

debug-only compilation bustage. opt tests look ok. Repushing debug:

https://tbpl.mozilla.org/?tree=Try&rev=bba4405e114a

Comment 54

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #53)
> debug-only compilation bustage. opt tests look ok. Repushing debug:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=bba4405e114a

Hm, looks _almost_ green, except for _one_ mochitest-o run where the tests for this bug (test_scriptSettings.xul) timed out.

I added some more diagnostics, and let's see if this reproduces with any amount of retriggering:

https://tbpl.mozilla.org/?tree=Try&rev=4e1344314441

Comment 55

[Bobby Holley (:bholley)]

Ok, with all that retriggering I can't reproduce the one test_scriptSettings.xul timeout from comment 53. So I'm going to just push this, with the extra diagnostics intact. If it ever goes orange on tinderbox, the log will presumably give us the information we need. :-)

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c84430c040dd

Comment 56

[Bobby Holley (:bholley)]

And...needs a clobber on windows: https://hg.mozilla.org/integration/mozilla-inbound/rev/c76cf8b7ae40

Comment 57

[Carsten Book [:Tomcat]]

had to backout this changes in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9a92e42151dc because of bustage on windows xp builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=31545296&tree=Mozilla-Inbound

Comment 58

[Bobby Holley (:bholley)]

Looks like the winXP opt compiler (and that compiler only) requires an argument to MOZ_ASSUME_UNREACHABLE.

https://tbpl.mozilla.org/?tree=Try&rev=8ec945afa227

Comment 59

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=e38e402fa80c

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf2019278b77
https://hg.mozilla.org/mozilla-central/rev/1c03be82647e
https://hg.mozilla.org/mozilla-central/rev/c95c51d24d1c
https://hg.mozilla.org/mozilla-central/rev/82df7174496c
https://hg.mozilla.org/mozilla-central/rev/a6b672cbd54d
https://hg.mozilla.org/mozilla-central/rev/c5940c217bd9
https://hg.mozilla.org/mozilla-central/rev/7be223bdc61a
https://hg.mozilla.org/mozilla-central/rev/d8a07b335193
https://hg.mozilla.org/mozilla-central/rev/6c504f06a0fb
https://hg.mozilla.org/mozilla-central/rev/97e479dec2cf
https://hg.mozilla.org/mozilla-central/rev/35371620801a
https://hg.mozilla.org/mozilla-central/rev/e38e402fa80c

Comment 61

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/b50d803d0ad5 - apparently that gaia-ui-test failure in the try push in comment 52 was real, and virtually permanent (though not, inconveniently, permanent enough to have hit the one push on inbound that we decided to merge, so you wound up merged to every tree and busting every tree).

Comment 62

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I'm currently in the process of trying to get some actual directions for running the gaia UI tests out of the people who created them, for bug 697343.  Hopefully once we have that we can see what's going on...

Comment 63

[Bobby Holley (:bholley)]

Bisecting the gaia-ui tests on try:

https://tbpl.mozilla.org/?tree=Try&rev=56c2dad4da11
https://tbpl.mozilla.org/?tree=Try&rev=f6eee244bff8
https://tbpl.mozilla.org/?tree=Try&rev=43a802a08343

Comment 64

[Boris Zbarsky [:bzbarsky]]

There are slightly better directions for running those tests locally at https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/gaia-ui-tests/Gaia_UI_Tests_Run_Tests#Testing_locally_with_TBPL_configuration_%28for_Gecko_and_Gaia_developers%29 now...

Comment 65

[Bobby Holley (:bholley)]

Bisecting further given the results from comment 63:

https://tbpl.mozilla.org/?tree=Try&rev=68af48ac6256
https://tbpl.mozilla.org/?tree=Try&rev=1b4ce1e405e0
https://tbpl.mozilla.org/?tree=Try&rev=48254a3fc029

Comment 66

[Steve Fink [:sfink] [:s:]]

Attachment: Root around GC call GetIncumbentGlobal,

Comment 68

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8345480 [details] [diff] [review]
Root around GC call GetIncumbentGlobal,

r=bz from bug 947555

Comment 69

[Bobby Holley (:bholley)]

comment 65 indicates that part 2 is the culprit. Nice.

So I just sunk some time into trying to reproduce this locally. After a lot of fiddling I finally managed to run the gaia-ui tests. Unfortunately, while there _are_ various failure, I think they're related to the differences in using a debug build on OSX (as opposed to an opt build on linux64). Getting rid of --restart helps a fair amount, but the actual socket-related timeouts from TBPL [1] still don't reproduce.

jgriffin and I talked about this on IRC. As it turns out, this is _already_ an existing intermittent failure (bug 948395), and these patches just seem to be tickling it to make it a bit less intermittent (though not fully permanent, see comment 61). And given the nature of patch 2, and the fact that everything else on CI is green, I'm near-certain that it's the b2g automation that's going to end up needing to change.

I think it makes sense for someone on the a-team to continue the investigation at this point. Zac, can you take the lead? This is rather high-priority stuff from the platform and security side.

[1] https://tbpl.mozilla.org/?tree=Try&rev=48254a3fc029

Comment 70

[Zac C (:zac)]

This Gu intermittent in bug 948395 was only brought to my attention Friday last week so I'm not entirely convinced you're in the clear.

I will run the 48254a3fc029 build locally today and let you know.

Comment 71

[Zac C (:zac)]

This build is causing b2g to crash.

The Crash Reporter pops up and the socket times out because the b2g process us not running anymore.

I filed the crash reports:
https://crash-stats.mozilla.com/report/index/38d54d4b-302c-416e-9c7b-deac02131211
https://crash-stats.mozilla.com/report/index/57f37354-36da-42a9-b391-c21f72131211



ROUND 0
-------
starting httpd
running webserver on http://10.246.27.126:51141/
TEST-START test_settings.py
test_get_all_settings (test_settings.TestSettings) ... ok
test_set_named_setting (test_settings.TestSettings) ... ERROR
test_set_volume (test_settings.TestSettings) ... ERROR

======================================================================
ERROR: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 127, in run
    self.setUp()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 814, in setUp
    self.cleanup_gaia(full_reset=False)
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 882, in cleanup_gaia
    self.apps.kill_all()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 118, in kill_all
    self.marionette.execute_async_script("GaiaApps.killAll()")
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 1073, in execute_async_script
    filename=os.path.basename(frame[0]))
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 567, in _send_message
    raise TimeoutException(message='socket.timeout', status=ErrorCodes.TIMEOUT, stacktrace=None)
TEST-UNEXPECTED-FAIL | test_settings.py test_settings.TestSettings.test_set_named_setting | TimeoutException: socket.timeout
======================================================================
ERROR: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 127, in run
    self.setUp()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 773, in setUp
    MarionetteTestCase.setUp(self)
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 312, in setUp
    CommonTestCase.setUp(self)
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 247, in setUp
    self.marionette.start_session()
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 681, in start_session
    self.session = self._send_message('newSession', 'value')
  File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette.py", line 567, in _send_message
    raise TimeoutException(message='socket.timeout', status=ErrorCodes.TIMEOUT, stacktrace=None)
TEST-UNEXPECTED-FAIL | test_settings.py test_settings.TestSettings.test_set_volume | TimeoutException: socket.timeout
----------------------------------------------------------------------
Ran 3 tests in 494.882s

FAILED (errors=2)

Comment 72

[Boris Zbarsky [:bzbarsky]]

> I filed the crash reports:

Those seem to have no useful symbols.  :(  Was this a local build of b2g that crash-stats would know nothing about?

The only useful thing in there is that we seem to think it's a null-deref...

Comment 73

[Zac C (:zac)]

Boris it was the Try build from here: https://tbpl.mozilla.org/?tree=Try&rev=48254a3fc029 so I guess that crash-stats would not be familiar.

I unpacked that, then I built a Gaia profile using a commit around the time that the Try build was made.

I ran the tests with the Try binary with the Gaia profile together.

I didn't do any manual exploratory testing to see if I could trigger the crash.

Comment 74

[Boris Zbarsky [:bzbarsky]]

Benjamin, is there a way to do a try b2g build that crash-stats would have symbols for?

Comment 75

[Benjamin Smedberg]

Well, not crash-stats, no. But the symbols are uploaded with the try results at http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-48254a3fc029/try-linux64_gecko/.

So if you have a minidump and those symbols, you can run minidump-stackwalk locally on it to get your report.

Comment 76

[Boris Zbarsky [:bzbarsky]]

Hmm.  How do I get a minidump?  Is that the "raw dump" option on crashstats?  I don't seem to have the bits to download those....

Comment 77

[Boris Zbarsky [:bzbarsky]]

Benjamin sent me the minidumps.

The stack looks like so:

 0  libxul.so!JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*) [Barrier.h:48254a3fc029 : 278 + 0x0]
    rbx = 0x00007f732514a140   r12 = 0x00007f730fe8a7a8
    r13 = 0x00007f72f19d4101   r14 = 0x0000000000000001
    r15 = 0x0000000000000000   rip = 0x00007f73232c4b6b
    rsp = 0x00007fff38741da8   rbp = 0x00007fff38741e40
    Found by: given as instruction pointer in context
 1  libxul.so!mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, bool, JSContext*) [Maybe.h:48254a3fc029 : 61 + 0xe]
    rbx = 0x00007f732514a140   r12 = 0x00007f730fe8a7a8
    r13 = 0x00007f72f19d4101   r14 = 0x0000000000000001
    r15 = 0x0000000000000000   rip = 0x00007f73226fa7d7
    rsp = 0x00007fff38741db0   rbp = 0x00007fff38741e40
    Found by: call frame info
 2  libxul.so!mozilla::dom::CallbackObject::CallSetup::CallSetup(JS::Handle<JSObject*>, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) [Maybe.h:48254a3fc029 : 68 + 0x15]
    rbx = 0x00007fff38741e30   r12 = 0x00007f732514a140
    r13 = 0x00007f72f19d4158   r14 = 0x0000000000000001
    r15 = 0x0000000000000000   rip = 0x00007f73225ae1b8
    rsp = 0x00007fff38741df0   rbp = 0x00007f730fe8a7a8
    Found by: call frame info
 3  libxul.so!mozilla::dom::EncodingCompleteEvent::Run() [HTMLCanvasElementBinding.h:48254a3fc029 : 112 + 0x4]
    rbx = 0x00007f72f7545a00   r12 = 0x00007f72f19d4140
    r13 = 0x00007fff38741f5f   r14 = 0x0000000000000001
    r15 = 0x0000000000000000   rip = 0x00007f73228716f4
    rsp = 0x00007fff38741e20   rbp = 0x00007f72f1a9a590
    Found by: call frame info

and above that is just event loop stuff.  Both of the stacks from comment 71 look like that.

Comment 78

[Boris Zbarsky [:bzbarsky]]

The relevant line in AutoEntryScript() is this:

>+  mAc.construct(aCx, aGlobalObject->GetGlobalJSObject());

I don't see an obvious way that aCx would be null at this point, but could aGlobalObject->GetGlobalJSObject() be null?

Comment 79

[(not currently active) Ted Mielczarek]

Note that if you push to try with a special patch applied you can also get actual debug symbols and attach a debugger:
https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_debug_symbols

Comment 80

[Boris Zbarsky [:bzbarsky]]

E.g. I suspect that at times nsInProcessTabChildGlobal::GetGlobalJSObject might return null (for a closed tab?).

Comment 81

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #80)
> E.g. I suspect that at times nsInProcessTabChildGlobal::GetGlobalJSObject
> might return null (for a closed tab?).

Yeah, that's probably likely. Maybe we should just bite the bullet and null-check the return value of GetGlobalJSObject() and bail, just like we do for non-current inners?

Comment 82

[Boris Zbarsky [:bzbarsky]]

Worth a shot, at least...

Comment 83

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=759cae801844

Comment 84

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #83)
> https://tbpl.mozilla.org/?tree=Try&rev=759cae801844

Youpie! This is green. I'll fold that patch into part 2 (bz, does comment 82 count as r+ on that change?), apply sfink's rooting patch, and do one final try push.

Comment 85

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f4e9834779ee

Comment 86

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=9bc6da580a39

Comment 87

[Boris Zbarsky [:bzbarsky]]

> does comment 82 count as r+ on that change?

Let's say yes.

Comment 88

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67ae84fa6807
https://hg.mozilla.org/mozilla-central/rev/02888ac9e221
https://hg.mozilla.org/mozilla-central/rev/14b3f894b6f7
https://hg.mozilla.org/mozilla-central/rev/bfb80727bffa
https://hg.mozilla.org/mozilla-central/rev/366a45b41539
https://hg.mozilla.org/mozilla-central/rev/dd2f364eb5d4
https://hg.mozilla.org/mozilla-central/rev/ab4c7f0f41a4
https://hg.mozilla.org/mozilla-central/rev/ac718a07e3b8
https://hg.mozilla.org/mozilla-central/rev/ad7ddc159137
https://hg.mozilla.org/mozilla-central/rev/669b8a742048
https://hg.mozilla.org/mozilla-central/rev/281184c3f2ee
https://hg.mozilla.org/mozilla-central/rev/e4441e59b5f2
https://hg.mozilla.org/mozilla-central/rev/9bc6da580a39