Closed Bug 1172010 Opened 7 years ago Closed 7 years ago

Toolbox no longer opens after devtools.reload()

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox41 affected, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: past, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

STR:

1. Open GCLI and enter "tools builtin"
2. Open the toolbox.
3. The toolbox opens an empty iframe and the following errors appear in the console:

JavaScript error: chrome://browser/content/devtools/theme-switching.js, line 45: TypeError: newThemeDef is null
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: tab is null
Full stack: Toolbox.prototype.selectTool@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1136:5
Toolbox.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:378:13
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:887:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:766:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:39
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:729:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:687:7
Toolbox.prototype.open/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:327:36

*************************
Actually the same thing happens when using |tools srcdir| and |tools reload|, so it's the devtools.reload() call that is messing things up.
Summary: Toolbox no longer opens after |tools builtin| → Toolbox no longer opens after devtools.reload()
Attached patch patch v1Splinter Review
Might be regressed from bug 986841.
But may be not just that one.

There is two issues:
1) Unload process work fine and unregister module.
   main.js correctly unregister tools and they disappear from the menu/shortcuts.
   But during reload we prevent reloading main.js so that tools aren't re-registered.
   That's because of the _mainid check:
   http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm#363
2) We are not calling main() again.
   I don't know if we used to do it, but we should.

Ideally we would move most of gDevtools.jsm to a regular module,
into main.js for ex, or new modules as dependency to main.js.
Then, I think it would be up to Loader.jsm to call main()
and Loader.jsm may be the only one JSM we end up with!
Also, this is quite too magical to merge main.js exports onto Loader.jsm `devtools`.
It may be clearer and more explicit to just expose main.js exports on `devtools`
and may be expose `require` as new Loader.jsm symbol.

So this patch isn't perfect but at least it make it work again,
so that we can start working on various bugs related to live coding on devtools codebase!
We also need to fix promise require path...
Here is a first alternative. As the overall plan is to only make devtools codebase hackable,
I think it makes more sense to make everyhing else being loaded from firefox ressources.
So here, we load promise module from firefox.
Otherwise, we can just fix the path in the meantime.
This is just a typo...
Attachment #8639735 - Flags: review+
Attachment #8639728 - Flags: review?(jwalker)
Attachment #8639731 - Flags: review+
Attachment #8639728 - Flags: review?(jwalker) → review+
Comment on attachment 8639731 [details] [diff] [review]
fix promises - Always load builtin Promise module

Ryan, I landed just the typo for now, but what do you think about that?
Attachment #8639731 - Flags: feedback?
About comment 2, I filled bug 1188405 and bug 1188401.
Assignee: nobody → poirot.alex
Comment on attachment 8639731 [details] [diff] [review]
fix promises - Always load builtin Promise module

I think the alternative seems good, since we're only focused on reloading DevTools modules anyway, and promise is not one of those.
Attachment #8639731 - Flags: feedback? → feedback+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.