Closed Bug 1303288 Opened 3 years ago Closed 3 years ago

The hot-reload add-on fails to reload components that use BrowserLoader and React

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: jsnajdr, Assigned: ochameau)

Details

Attachments

(2 files, 1 obsolete file)

STR:
1. Install the hot-reload addon (install.rdf in about:debugging)
2. Open the Performance tool (no need to do anything, just open it)
3. Ctrl-Alt-R to reload devtools.

Expected: the Performance tool reloads just fine
Actual: error is thrown from React during the reload:

TypeError: 'get window' called on an object that does not implement interface Window.  react.js:19569
TypeError: EventEmitter is undefined[Learn More]  performance-view.js:411:1
TypeError: EventEmitter is undefined[Learn More]  overview.js:423:1
TypeError: EventEmitter is undefined[Learn More]  toolbar.js:160:1
TypeError: EventEmitter is undefined[Learn More]  details-waterfall.js:252:1
TypeError: EventEmitter is undefined[Learn More]  details-js-call-tree.js:193:1
TypeError: EventEmitter is undefined[Learn More]  details-js-flamegraph.js:125:1
TypeError: EventEmitter is undefined[Learn More]  details-memory-call-tree.js:130:1
TypeError: EventEmitter is undefined[Learn More]  details-memory-flamegraph.js:121:1
TypeError: EventEmitter is undefined[Learn More]  details.js:263:1
TypeError: EventEmitter is undefined[Learn More]  recordings.js:225:1
TypeError: PerformanceController is undefined
Stack trace:
PerformancePanel.prototype.open<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/performance/panel.js:57:5
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1283:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1225:14
generateRequestMethods/</frontProto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1363:14
PerformanceFront<.connect<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/fronts/performance.js:36:28
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
TaskImpl.prototype._handleResultValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:387:7
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:319:13
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
Toolbox.prototype.initPerformance<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:2237:11
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
PerformancePanel.prototype.open<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/performance/panel.js:46:23
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1291:19
  Promise-backend.js:940
Weird it breaks only now, may be acorn wasn't used before in a common codepath.
acorn was still package in omni.jar differently than the sources are organized on fs.
For the hot reload, we have to ensure not doing any magic during packaging and
put the files in the jar file with the exact same tree of files.
Attachment #8791923 - Flags: review?(pbrosset)
Attachment #8791923 - Attachment is obsolete: true
Attachment #8791923 - Flags: review?(pbrosset)
Oops sorry, I meant to attach that to bug 1303268...
I'll look into that early next week if that's not fixed by this patch.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> I'll look into that early next week if that's not fixed by this patch.

I still get the same error (React can't find window) after applying this Acorn patch.
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
Whaa that was quite a quest to figure this one out.
I haven't figured out the precise reason why wrappers behaves differently, but that was a wrapper issue.
We don't have xray wrapper when using the addon... whereas we do in production codepath.
And having xrays is a very important when it comes to pass this value to sandboxPrototype (within browser-loader.js) otherwise we get these weird exceptions:
  TypeError: 'get window' called on an object that does not implement interface Window.

Passing `window` instead of `this` fixes this. In most cases it should be equivalent.
`window` is going to be the "global scope/window object" for the related document,
whereas `this` is the current scope, which, in most cases should be somewhat equivalent to window, but not always. And here, it is complex enough to behave differently in the hotreload addon case.

Patrick, I asked for your review, but do not hesitate to forward to someone else. jryans is a good candidate but on pto this week.
Just pushed eslint fixes.
Comment on attachment 8793348 [details]
Bug 1303288 - Force unloading all JSM when reloading devtools via the addon.

https://reviewboard.mozilla.org/r/80100/#review79002
Attachment #8793348 - Flags: review?(pbrosset) → review+
Attachment #8793349 - Flags: review?(pbrosset) → review?(jlong)
Comment on attachment 8793349 [details]
Bug 1303288 - Pass the window object instead of current scope to prevent wrapper issues when using the devtools reload addon.

https://reviewboard.mozilla.org/r/80102/#review79448

Looks good. I don't know why these used `this` before; I used `window` in the debugger like you do in this patch. Thanks.
Attachment #8793349 - Flags: review?(jlong) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a8ba181b2e1
Force unloading all JSM when reloading devtools via the addon. r=pbro
https://hg.mozilla.org/integration/autoland/rev/1aaa9010a9a1
Pass the window object instead of current scope to prevent wrapper issues when using the devtools reload addon. r=jlongster
https://hg.mozilla.org/mozilla-central/rev/7a8ba181b2e1
https://hg.mozilla.org/mozilla-central/rev/1aaa9010a9a1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.