Closed Bug 1561970 Opened 7 months ago Closed 6 months ago

BrowserTabParent receiveMessage throws errors because of gBrowser object defined in webext-panels.js and aboutaddonsCommon.js

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 unaffected, firefox68 unaffected, firefox69 verified, firefox70 verified)

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- verified
firefox70 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Keywords: regression)

Attachments

(1 file)

When a options_page embedded in the extension's about:addons details view or an extension sidebar is being created, the following errors are logged in the BrowserConsole:

JavaScript error: resource:///actors/BrowserTabParent.jsm, line 27: TypeError: gBrowser.announceWindowCreated is not a function
JavaScript error: resource:///actors/BrowserTabParent.jsm, line 32: TypeError: browser.ownerGlobal.gBrowserInit is undefined

It seems that these errors are not currently triggering any visible issues for the users, but they are already an "hint" that we should look into avoiding / fixing them soon.

webext-panels.js and aboutaddonsCommon.js are currently defining their "fake" gBrowser object to ensure that the tab modals (e.g. alert / confirm etc.) do work in these extension pages as they do when the extension page is loaded in a regular Firefox tab.

See Also: → 1555711

Hi Mike,
I was wondering what is your opinion about the preferable way to fix / prevent the BrowserTabParent.jsm errors mentioned in comment 0.

As briefly mentioned in comment 0, ideally we could likely remove the "fake" gBrowser objects (gBrowser defined in webext-panels.js and gBrowser defined in aboutaddonsCommon.js) if we are going to support this scenario (tab modals / prompts triggered by extension pages not part of a Firefox tab) in a more explicit and clean way as part of the changes needed to make RemotePrompt fission-compatible (Bug 1555711), but Bug 1555711 is currently marked as P3 and so I'm not sure if we can already evaluate this as a solution to this issue.

Otherwise, we would likely have to take the existence of these "fake" gBrowser objects into account in BrowserTabParent.jsm.

Flags: needinfo?(mconley)

I suspect this problem is going to crop up again and again. :/ Perhaps a middleware class (like I suggested in bug 1559456) will make this sort of thing more transparent for folks porting our actors.

When you have remote <browser/> elements nested within a non-remote <browser/> element, I've found the best thing to do is (in the parent process) to resolve the remote sub<browser/> to the top-level non-remote <browser/> by getting at its chromeEventHandler:

https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/browser/actors/ContextMenuParent.jsm#18-20

That'll take care of getting access top the top-level <browser/>.

I'm not sure this answers your question though - it sounds like you're already mocking out gBrowser's in these special cases... does what I indicate here help at all?

Flags: needinfo?(mconley) → needinfo?(lgreco)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)

I'm not sure this answers your question though - it sounds like you're already mocking out gBrowser's in these special cases... does what I indicate here help at all?

In cases like the ContextMenuParent, making the actor to deal with the "nested <browser/> elements" scenario internally looks like the right approach to me (and as an additional confirmation, it allowed us to remove the special handling for the context menu support in the extension pages that are not loaded in a regular browser tab).

We have mocked out gBrowser for similar reasons (the remaining one seems to be "support for the modals that should be opened when an extension calls window.alert or window.confirm etc."), and in the long run we would definitely love to get rid of those mocked gBrowser objects completely.

But in the meantime, for the specific case related to BrowserTabParent.jsm, based on its name it sounds like we may want to actually ignore the browser elements that are not related to tabs, is that right?

Flags: needinfo?(lgreco) → needinfo?(mconley)

(In reply to Luca Greco [:rpl] [:luca] from comment #3)

But in the meantime, for the specific case related to BrowserTabParent.jsm, based on its name it sounds like we may want to actually ignore the browser elements that are not related to tabs, is that right?

Yes, that's correct.

Flags: needinfo?(mconley)
Assignee: nobody → lgreco
Priority: -- → P1
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/e3b76229e74f
BrowserTabParent should ignore browser elements related to non-tab extension pages. r=mconley
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Can you please provide some steps to manually verify this issue? If no manual testing is needed please mark this bug as "qe-verify-"

Flags: needinfo?(lgreco)

Please nominate this for Beta approval too.

(In reply to Madalin Cotetiu from comment #8)

Can you please provide some steps to manually verify this issue? If no manual testing is needed please mark this bug as "qe-verify-"

STR:

  • open the browser console
  • install an extension which includes a sidebar and/or a browserAction popup and/or an options page
  • open the sidebar (or browserAction popup, or the options page)
  • Expected behavior on builds including the fix: none of the BrowserTabParent.jsm errors mentioned in comment 0 have been logged in the Browser Console
  • Actual behavior on build non-including the fix: the BrowserTabParent.jsm errors mentioned in comment 0 have been logged in the Browser Console
Flags: needinfo?(lgreco)

Tested and reproduced the issue in Beta 69.0b5 (20190715173502), as expected.

Verified fix on Windows 10 64-Bit and Ubuntu 18.04.2 LTS 64-bit on Nightly 70.0a1 (20190717215119) using the provided steps to confirm that the BrowserTabParent.jsm errors are no longer encountered in the following cases:

  1. Installing and using a sidebar extension
  2. Performing a browser Action pop up
  3. Installing an extension with an Options page and navigating to it.

No regression encountered in the above flows or when navigating through the extension's detail pages.
Tested extensions: Tree Style Tab and HTTPS Everywhere.

Status: RESOLVED → VERIFIED

Comment on attachment 9076181 [details]
Bug 1561970 - BrowserTabParent should ignore browser elements related to non-tab extension pages. r?mconley!

Beta/Release Uplift Approval Request

  • User impact if declined: None.
    Some errors will be logged in the browser console when a non-tab extension page is being created.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Same STR used to verify the fix in Nightly (Bug 1561970 comment 10).
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change shouldn't be a risky one: it is a small change that makes BrowserTabParent's receiveMessage method to return earlier when it detects that the <browser/> element from which a message has been received is not related to a browser tab.
  • String changes made/needed:
Attachment #9076181 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9076181 [details]
Bug 1561970 - BrowserTabParent should ignore browser elements related to non-tab extension pages. r?mconley!

Cleans up some spurious log spam. Approved for 69.0b6.

Attachment #9076181 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Verified fix on Windows 10 64-Bit and Ubuntu 18.04.2 LTS 64-bit on Beta 69.0b6 with Build ID: 20190718172058 using the provided steps with the Tree Style Tab and the HTTPs extensions.

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