1494009 - about:addons expects its disco pane content browser to have a non-null currentURI when _loadURI gets called

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[:Gijs (he/him)]

e.g. https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=201419084&revision=aa93743aa6b57f7922aa0f17ace8f6294fea812b

From IRC:

16:37:44 <@Gijs> aswan: the same-process browser binding implementation touches browser.securityUI in the constructor
16:37:47 <@Gijs> that touches browser.contentWindow
16:37:55 <@Gijs> that forces the creation of a dummy about:blank document
16:38:04 <@Gijs> I'm trying to fix this in bug 1493655
16:38:05 <firebot> https://bugzil.la/1493655 — ASSIGNED, [e-mail] — nsISecureBrowserUI::Init should take a docshell instead of a content window
16:38:13 <@Gijs> https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=201419128&revision=aa93743aa6b57f7922aa0f17ace8f6294fea812b
16:38:45 <@Gijs> specifically, the _loadURL call in extensions.js errors out now because the browser's currentURI hasn't been set, because nothing is loaded in the docshell
16:39:13 — @Gijs expects docshell just returns null or whatever
16:39:45 <@Gijs> obviously this is fixable, but I dunno how long this particular tail is.
16:40:08 <aswan> yuck
16:40:12 <@Gijs> also not 100% sure what the right fix is... I guess making the about:addons code cope with a null currentURI
16:40:19 <aswan> i was going to say, from a small sampling, it looks like that one place is responsible for many failures
16:40:37 <@Gijs> but I don't know if _loadURI is the only place

I currently intend to just fix this in the about:addons code with a dummy browser.contentWindow; fetch, and a reference to this bug. We can fix it properly here - likely we'll just need to be OK with currentURI being null, but I don't know how deep the rabbithole goes, so wanted to split it out to its own bug.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1494009 Guard against null currentURI in extensions.js

Comment 2

[Andrew Swan [:aswan]]

There are several other references to this._browser.currentURI in gDiscoverView, all of which account for it being null.  Attached is a trivial patch to check it here as well, if I misunderstood your comment about the rabbit hole and am overlooking some greater complexity, apologies...

(also, I don't think its practical to add a new test specifically for this patch but its also trivial enough to just inspect manually, and ideally we'll start exercising the new code path soon enough when your bug 1493655 patches land)

Comment 3

[:Gijs (he/him)]

Comment on attachment 9012032 [details]
Bug 1494009 Guard against null currentURI in extensions.js

:Gijs (out Thu 27 - Sun 30 / 9;  he/him) has approved the revision.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cdfaafc3e6b
Guard against null currentURI in extensions.js r=Gijs

Comment 5

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0cdfaafc3e6b