527784 - Browser code incorrectly looking for browser-only modules in gre location

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Reed Loden [:reed]]

Attachment: patch - v1 (trunk/1.9.2)

A member of the Ubuntu Mozilla Team noticed that his sessions were no longer being restored, and he noticed the following error in his Error Console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://browser/content/browser.js :: delayedStartup :: line 3590"  data: no]

browser.js line 3590 is:
  Cu.import("resource://gre/modules/NetworkPrioritizer.jsm", NP);

When he went to resource://gre/modules/NetworkPrioritizer.jsm, he got file not found. However, the file was under /usr/lib/firefox-3.7a1pre/modules/ along with distribution.js and openLocationLastURL.jsm.

Mook pointed out that such import code should probably be using resource://app/ instead of resource://gre/. Indeed, resource://app/modules/NetworkPrioritizer.jsm works just fine.

This patch modifies the file paths for all calls to distribution.js, NetworkPrioritizer.jsm, and openLocationLastURL.jsm.

Requesting blocking, as this currently makes session restore (at least) completley broken.

Comment 1

[Reed Loden [:reed]]

This also affects 1.9.1 because of private browsing's openLocationLastURL.jsm, though the same problems aren't seen.

Comment 2

[Mike Hommey [:glandium]]

resource:///modules/NetworkPrioritizer.jsm would work equally well

Comment 3

[Reed Loden [:reed]]

Attachment: patch - v1 (1.9.1)

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 411527 [details] [diff] [review]
patch - v1 (1.9.1)

>-Cu.import("resource:///modules/distribution.js");
>+Cu.import("resource://app/modules/distribution.js");

This is not required.

Comment 5

[Reed Loden [:reed]]

(In reply to comment #4)
> (From update of attachment 411527 [details] [diff] [review])
> >-Cu.import("resource:///modules/distribution.js");
> >+Cu.import("resource://app/modules/distribution.js");
> 
> This is not required.

It's not required, no, but explicitly saying 'app' is way more clearer than wondering what /// means.

Comment 6

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/3c230b561fa2

Comment 7

[Mike Hommey [:glandium]]

(In reply to comment #5)
> It's not required, no, but explicitly saying 'app' is way more clearer than
> wondering what /// means.

There are more places to change, then.

Comment 8

[Reed Loden [:reed]]

(In reply to comment #7)
> (In reply to comment #5)
> > It's not required, no, but explicitly saying 'app' is way more clearer than
> > wondering what /// means.
> 
> There are more places to change, then.

The only other place (that's not part of some test or comment) is http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeRegistry.cpp#1140, which is:

NS_LITERAL_CSTRING("resource:///chrome/app-chrome.manifest"));

I could change that, I guess, but I was mostly caring about the three files under modules. Are you aware of any other places using resource:/// that actually matter?

Comment 9

[Mike Hommey [:glandium]]

browser/base/content/pageinfo/pageInfo.xul
and possibly chrome/public/nsIToolkitChromeRegistry.idl

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> browser/base/content/pageinfo/pageInfo.xul
> and possibly chrome/public/nsIToolkitChromeRegistry.idl

You must be looking at older code. Trunk (mozilla-central) doesn't have resource:/// in either of those files.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 411523 [details] [diff] [review]
patch - v1 (trunk/1.9.2)

a192=beltzner

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The risk here is low - it can only affect builds where mGREDir != mXULAppDir, which isn't the case for stock Firefox (only affects firefox-on-xulrunner). In our builds, these URLs are all equivalent.

Given the benefit to distros and the lack of risk, I think we should take this for 1.9.1.6.

Comment 13

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1316e9a4b655

Comment 14

[Daniel Veditz [:dveditz]]

This is _so_ not a 1.9.1 blocker. These files can't have changed in forever on the 1.9.1 branch, how can it suddenly be a "blocking" issue the day of the code freeze?

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 411527 [details] [diff] [review]
patch - v1 (1.9.1)

Approved for 1.9.0.16, a=dveditz for release-drivers (for the next 6 or so hours until code-freeze, then it'll have to wait until next time).

Comment 16

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/672c7b2cf968

Comment 17

[Phil Ringnalda (:philor)]

So, you've got unstarred (other than the ones we did wrongly out of confusion) test failures across all three branches, after your second set of pushes - do you have a plan?

Comment 18

[Samuel Sidler (old account; do not CC)]

(In reply to comment #15)
> (From update of attachment 411527 [details] [diff] [review])
> Approved for 1.9.0.16

Dan meant 1.9.1.6 here.

If the test failures were caused by this bug, please back out immediately.

Comment 19

[Reed Loden [:reed]]

Turns out that xpcshell doesn't implement resource://app/ like toolkit's nsXREDirProvider does, so just made everything use resource:///.

Related commits:
http://hg.mozilla.org/mozilla-central/rev/8f50b27ebb3c - fix test
http://hg.mozilla.org/mozilla-central/rev/baf856d402c6 - fix everything else
http://hg.mozilla.org/mozilla-central/rev/3c6b5f5ef7d6 - revert accidental change
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ffb934882c61 - fix test
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1c65e2a65421 - fix everything else
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6a476c4cddbf - fix test
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b0b40bb0f38a - fix everything else

Summary of changes to original patch:
* Revert browser/components/nsBrowserGlue.js change (as it was already resource:///)
* Swap resource://app/ to resource:/// for all other import calls

Comment 20

[Frederic Bezies]

Could it produce this error while trying to build trunk source code ?

nsISemanticUnitScanner.idl
/home/fred/logs/fox/src/intl/lwbrk/idl/nsISemanticUnitScanner.idl:66: unterminated comment
input callback returned failure
make[4]: *** [_xpidlgen/nsISemanticUnitScanner.h] Error 2
make[3]: *** [export] Error 2
make[2]: *** [export] Error 2
make[1]: *** [export] Error 2
make: *** [depend] Error 2

Comment 21

[:Gavin Sharp [email: [e-mail]]]

no

Comment 22

[Al Billings [:abillings - ex-MoCo]]

It isn't clear how to replicate this problem on Ubuntu in order to test the fixed builds.

Comment 23

[Reed Loden [:reed]]

(In reply to comment #22)
> It isn't clear how to replicate this problem on Ubuntu in order to test the
> fixed builds.

The problem can't be found in mozilla.org builds due to the lack of XULRunner use, so gre == app. Only Ubuntu (and related Linux distro) builds will exhibit this problem because of how they build XULRunner (gre) as a separate component and then run Firefox (app) on top of it (causing gre != app).