805585 - Call to non-existent browserFrame.contentWindow in shell.js

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=798580#c10

It seems like nothing past this point is ever getting executed as an exception is thrown. (this is preventing us from loading a custom browser.homescreenURL that we set in some user prefs)

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Remove erroneous code block

Giving review to :jlebar as he noticed the issue initially

Comment 2

[Andrew Halberstadt [:ahal]]

Requesting blocking basecamp as this blocks bug 798580 which was +'ed

Comment 3

[Justin Lebar (not reading bugmail)]

> It seems like nothing past this point is ever getting executed as an exception is thrown.

That's true in the out-of-process case.  But we established that this is an in-process iframe in normal B2G operation.

I think this should be fine because of (b) in bug 798580 comment 10.  But I'd like someone who knows this code to look at this patch.

Comment 4

[Andrew Halberstadt [:ahal]]

(In reply to Justin Lebar [:jlebar] from comment #3)
> That's true in the out-of-process case.  But we established that this is an
> in-process iframe in normal B2G operation.

Oh, this makes so much more sense now.

In our automation code we were setting dom.ipc.browser_frames.oop_by_default to true (perhaps we shouldn't be doing this). We then restart B2G and an exception gets thrown preventing the onload listener from being registered here: http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#198

So landing this patch should fix the problem in bug 798580. Not sure if this would be good enough or if I should still go ahead and make mochitests run from inside a shim app though.

Comment 5

[Justin Lebar (not reading bugmail)]

> So landing this patch should fix the problem in bug 798580.

No, I don't think so.  You'd have to make shell.xul completely oop-safe, which means removing every reference to contentWindow, changing all "load" events to "mozbrowserloadend", and probably other things.

Unless this is the only place that shell.xul touches contentWindow.

Comment 6

[Andrew Halberstadt [:ahal]]

Un-noming due to comment 5 and https://bugzilla.mozilla.org/show_bug.cgi?id=798580#c51

Looks like this might still be something that should be landed, but there probably isn't a huge point. Our automation shouldn't be setting dom.ipc.browser_frames.oop_by_default.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 675262 [details] [diff] [review]
Patch 1.0 - Remove erroneous code block

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

Clearing since this does not look like it's really needed.

Comment 8

[Andrew Halberstadt [:ahal]]

Not sure if this is even still an issue.