943681 - Convert to Promise.jsm in the webconsole

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

Converts everything devtools/webconsole to use Promise.jsm. https://tbpl.mozilla.org/?tree=Try&rev=86f6daf585c0

Comment 2

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

Change the lazy getters to lazy importers.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

Woops, I forgot that you can't use lazyImporter here (because of the "Promise" -> "promise" rename). This should work. https://tbpl.mozilla.org/?tree=Try&rev=10bd75c2f9d5

Comment 4

[Mihai Sucan [:msucan]]

The lazyImporter() can take a fourth argument which solves the problem you had with it. See XPCU_defineLazyModuleGetter() in js/xpconnect/loader/XPCOMUtils.jsm.

(you can leave it as-is with the lazyGetter, just mentioning that nicety of XPCOMUtils)

Comment 5

[Brandon Benvie [:benvie]]

I'm fairly sure this is ready to land (and not stirring up some crazy intermittents). I'll get this landed ASAP (with Mihai's feedback).

Comment 6

[Mihai Sucan [:msucan]]

Is this patch waiting on me for landing? I hope not. Feel free to land it - I don't expect rebase issues with my patches for bug 843004.

Comment 7

[Brandon Benvie [:benvie]]

I think that bc failure might be permaorange because of this patch. https://tbpl.mozilla.org/?tree=Try&rev=0862711cee75

Comment 8

[Brandon Benvie [:benvie]]

Trying again. https://tbpl.mozilla.org/?tree=Try&rev=18346611b6f6

Comment 9

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

I think this will fix it. https://tbpl.mozilla.org/?tree=Try&rev=3ff4324b4872

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 8346841 [details] [diff] [review]
promise-webconsole.patch

Looks good. Thanks!

Comment 11

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/86a9b2253199

Comment 12

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out:
https://hg.mozilla.org/integration/fx-team/rev/6639771d3626

because of timeouts in browser_toolbox_raise.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31950694&tree=Fx-Team

(This was previously an intermittent failure, but it became perma-orange on OS X after this patch.)

Comment 13

[Mihai Sucan [:msucan]]

Sorry for bothering. Brandon, any progress with this patch? Can I help with anything?

Comment 14

[Brandon Benvie [:benvie]]

Thanks Mihai. I don't think there's a problem with the webconsole at all actually. The test (framework/test/browser_toolbox_raise.js) has an intermittent which this patch turns permanently orange on OS X. The test has some fiddly bits relating to window focus which seems to be sensitive to the timing changes. 

Do you have any idea on how to fix the test to not be sensitive to window focus/timing interaction? I think fixing this bug just hinges on that, and not any more changes to the webconsole or anything else.

Comment 15

[Mihai Sucan [:msucan]]

Brandon, maybe try to remove executeSoon() when it is not needed. There are two calls to the function, see if they can be replaced with event listeners (tabselected).

TBPL shows that these failures happen only with Mac OSX. Can you reproduce? According to tbpl, the timeouts occur after the test finished (!). See:

https://tbpl.mozilla.org/php/getParsedLog.php?id=31948868&tree=Fx-Team#error0

12:18:15     INFO -  ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID

This error seems to suggest that cleanup() and finish() are called before the toolbox finishes executing its own code. Try to get out of the execution loop with the infamous executeSoon(cleanup). Also, after you call toolbox.destroy(), maybe executeSoon(finish) would help.

I hope this helps.

(I pinged about this patch simply because exceptions continue to not show in our devtools. Promise.jsm is very useful for development. Thank you!)

Comment 16

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

This patch is also now disabling the test browser_devtools_raise.js. That test is blocking a couple of bugs and already has intermittent issues, so it needs to be fixed (bug 962258). With the test disabled this patch should be good to go.

Comment 17

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

Comment 18

[Brandon Benvie [:benvie]]

Comment on attachment 8363237 [details] [diff] [review]
promise-webconsole.patch

Verbal r+ from dcamp on disabling the test.

Comment 19

[Brandon Benvie [:benvie]]

Attachment: promise-webconsole.patch

Disabling test was moved to its own bug and landed. Seems something changed to introduced another intermittent and I've added an attempt to fix it.

Comment 20

[Brandon Benvie [:benvie]]

Comment on attachment 8363800 [details] [diff] [review]
promise-webconsole.patch

It's looking good. Mihai, could you just take another look at the change I made to browser_console_dead_objects.js?

Comment 21

[Brandon Benvie [:benvie]]

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

Comment 22

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/f63078a8ce74

Comment 23

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

Backed out in https://hg.mozilla.org/integration/fx-team/rev/535f0a313b28 for introducing some webconsole test timeouts on at least Windows Debug builds:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33492261&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=33490982&tree=Fx-Team

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/f63078a8ce74

Comment 25

[Mihai Sucan [:msucan]]

(This is not fixed. It's backed-out.)

Comment 26

[Carsten Book [:Tomcat]]

(In reply to Wes Kocher (:KWierso) from comment #24)
> https://hg.mozilla.org/mozilla-central/rev/f63078a8ce74

note that this happened because a merge cset with this fix included landed on central and so this got set fixed. Merged now a cset with the backout, so this also backed out from m-c and all the other branches (tl;dr msucan is right this is not fixed :)

Comment 27

[Brandon Benvie [:benvie]]

Bug 963869 is about browser_console_dead_objects.js. It came up shortly after this landed and is one of the two failures that caused the backout. The intermittent persisted after this patch was backed out which makes me think it wasn't related to this patch. The test was recently disabled due to too many failures.

Comment 28

[Mihai Sucan [:msucan]]

Brandon, given the test is disabled, can you please push this patch to try server, then to fx-team if it's fine? I'll check what's the issue with that test...

Comment 29

[Mihai Sucan [:msucan]]

I wanted to pick up this patch to get it landed. However, I see the patch *is* landed.

Also, hg blame points to the changeset 165092:3cf6fcb75118 ("Merge mozilla-central to mozilla-inbound"). When did this land?

Comment 30

[Brandon Benvie [:benvie]]

It was originally landed January 24. I thought it was backed out shortly after but apparently not? I haven't attempted to re-land it.

Comment 31

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #30)
> It was originally landed January 24. I thought it was backed out shortly
> after but apparently not? I haven't attempted to re-land it.

This is weird. Carsten confirmed in comment #26 this patch was backedout, however, I now see it's not. Please confirm and we can mark the bug as fixed. I am glad the patch is in. :)

Comment 32

[Brandon Benvie [:benvie]]

I can confirm that the changes are indeed actually there (e.g. Promise.jsm is imported at the top of webconsole.js). I guess since it's been in for a month and we've handled any fallout from it, we might as well keep it in.