Closed Bug 1303288 Opened 8 years ago Closed 8 years ago

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

Tracking

(firefox52 fixed)

Status:

RESOLVED FIXED

Milestone:

Firefox 52

Tracking Flags:

Tracking

Status

firefox52

---

fixed

People

(Reporter: jsnajdr, Assigned: ochameau)

Details

Attachments

(2 files, 1 obsolete file)

Package acorn lib with same path as sources to fix hot reload. 8 years ago Alexandre Poirot [:ochameau] 2.39 KB, patch		Details \| Diff \| Splinter Review
Bug 1303288 - Force unloading all JSM when reloading devtools via the addon. 8 years ago Alexandre Poirot [:ochameau] 58 bytes, text/x-review-board-request	pbro : review+	Details
Bug 1303288 - Pass the window object instead of current scope to prevent wrapper issues when using the devtools reload addon. 8 years ago Alexandre Poirot [:ochameau] 58 bytes, text/x-review-board-request	jlong : review+	Details

Jarda Snajdr [:jsnajdr]

Reporter

Description

•

8 years ago

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

Alexandre Poirot [:ochameau]

Assignee

Comment 1

•

8 years ago

Attached patch Package acorn lib with same path as sources to fix hot reload. (obsolete) — Details — Splinter Review

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)

Alexandre Poirot [:ochameau]

Assignee

Comment 2

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76596a1fa6ab

Alexandre Poirot [:ochameau]

Assignee

Updated

•

8 years ago

Attachment #8791923 - Attachment is obsolete: true

Attachment #8791923 - Flags: review?(pbrosset)

Alexandre Poirot [:ochameau]

Assignee

Comment 3

•

8 years ago

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)

Jarda Snajdr [:jsnajdr]

Reporter

Comment 4

•

8 years ago

(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.

Alexandre Poirot [:ochameau]

Assignee

Updated

•

8 years ago

Assignee: nobody → poirot.alex

Flags: needinfo?(poirot.alex)

Alexandre Poirot [:ochameau]

Assignee

Comment 5

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=682ca2452241

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/80100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80100/

Comment hidden (mozreview-request)

If we happen to pass the current scope (`this`), the browser-loader codebase fails
when passing it as sandboxPrototype. It only works when passing an xray wrapper.
The document scope happen to not be an xray when using the addon, whereas `window`
always is no matter if we are using the addon or not.

Review commit: https://reviewboard.mozilla.org/r/80102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80102/

Alexandre Poirot [:ochameau]

Assignee

Comment 8

•

8 years ago

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.

Comment hidden (mozreview-request)

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80102/diff/1-2/

Alexandre Poirot [:ochameau]

Assignee

Comment 10

•

8 years ago

Just pushed eslint fixes.

Patrick Brosset <:pbro>

Comment 11

•

8 years ago

mozreview-review

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+

Patrick Brosset <:pbro>

Updated

•

8 years ago

Attachment #8793349 - Flags: review?(pbrosset) → review?(jlong)

James Long (:jlongster)

Comment 12

•

8 years ago

mozreview-review

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+

Pulsebot

Comment 13

•

8 years ago

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

Carsten Book [:Tomcat]

Comment 14

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/7a8ba181b2e1
https://hg.mozilla.org/mozilla-central/rev/1aaa9010a9a1

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox52: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 52

BMO Automation

Updated

•

6 years ago

Product: Firefox → DevTools

You need to log in before you can comment on or make changes to this bug.