Closed Bug 1296589 Opened 4 years ago Closed 4 years ago

TypeError: this._locationStore is null when navigating from about:newtab with DevTools open

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jsnajdr, Assigned: jbhoosreddy)

References

Details

Attachments

(1 file, 2 obsolete files)

Steps to reproduce:
1. Open about:newtab
2. Open DevTools on the about:newtab page
3. Navigate to another page in the same tab (e.g., by clicking on one of the tiles offered by about:newtab)

Result:
TypeError: this._locationStore is null: SourceMapService.prototype.reset@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/source-map-service.js:44:3
SourceMapService.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/source-map-service.js:49:3
Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:2060:7
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:191:13
TabTarget.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:567:5
TabTarget.prototype._setupRemoteListeners/this._onTabDetached@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:481:9
eventSource/aProto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:132:9
DebuggerClient.prototype.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1007:7
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Tentatively blocking on the source maps in console bug since I'm assuming that's when this started
Blocks: 670002
Apparently, I'm unable to assign myself to this bug, but I can take this.
Attached patch 1296589.patch (obsolete) — Splinter Review
Added guards to reset and unsubscribe methods for the source map service to prevent access to the store, if it is not initialized yet.
Attachment #8783066 - Flags: review?(jsantell)
Assignee: nobody → jaideepb
Status: NEW → ASSIGNED
Comment on attachment 8783066 [details] [diff] [review]
1296589.patch

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

Looks good -- I wonder if we should just clear the map upon destroy rather than having these checks and nullifying the maps.
Attachment #8783066 - Flags: review?(jsantell) → review+
There might be a better way to fix this. The error stack trace suggests that the sourceMapService.destroy() method is called on already destroyed (or unitialized) instance. Jaideep's patch adds guards in this method, effectively hiding the error instead of really fixing it. Jaideep, maybe you could investigate what is going on and why the incorrect calls happen? This would lead to a fix that takes more effort, but ultimately leads to cleaner and better code.
(In reply to Jarda Snajdr [:jsnajdr] from comment #5)
> There might be a better way to fix this. The error stack trace suggests that
> the sourceMapService.destroy() method is called on already destroyed (or
> unitialized) instance. Jaideep's patch adds guards in this method,
> effectively hiding the error instead of really fixing it. Jaideep, maybe you
> could investigate what is going on and why the incorrect calls happen? This
> would lead to a fix that takes more effort, but ultimately leads to cleaner
> and better code.

Jordan and I discussed about this before, and this is one of the things I have to investigate, in one of my follow up bugs. I believe I already have a follow up bug filed for the issue of handling toolbox events, where I was planning on looking into this more deeply, as I go through my notes when I go back. Personally, I would like land this patch so that this error does not add noise for others trying out the feature. But if a little delay is okay, I can resolve this in this bug itself. I am planning to tackle the root cause of the issue though.

(In reply to Jordan Santell [:jsantell] [@jsantell] (Away from Bugzilla for awhile) from comment #4)
> Comment on attachment 8783066 [details] [diff] [review]
> 1296589.patch
> 
> Review of attachment 8783066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good -- I wonder if we should just clear the map upon destroy rather
> than having these checks and nullifying the maps.

I still have to try it out but I feel like you are right about not nullifying the Map objects. I looked into the toolbox events which the source map service listens to, and I think I understand why we were seeing that behavior. I will look into it and submit another patch if it works better.
(In reply to Jaideep Bhoosreddy [:jbhoosreddy] from comment #2)
> Apparently, I'm unable to assign myself to this bug, but I can take this.

(I have given your new Bugzilla account permission to make changes.)
Attached patch 1296589.patch [1.0] (obsolete) — Splinter Review
Removed listener in the Source Map Service for the toolbox "close" event, as it is handled by the toolbox itself, which explicitly calls the destroy method for this.
Attachment #8783066 - Attachment is obsolete: true
Attachment #8784104 - Flags: review+
Attachment #8784104 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/cb2e635542cf
Add guard to prevent accessing location store before it is initialized. r=jsantell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb2e635542cf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this bug with Firefox nightly 51.0a1(build id:20160819030226)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 51.0b8(build id:20161215085501)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20161214]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.