746457 - Trunk Hourly disables Adblock Plus and No Script

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Leman Bennett [Omega]]

http://hg.mozilla.org/mozilla-central/rev/93dfd98900ad

For whatever odd reason, the recent hourly build forcefully disables Adblock Plus, all versions including the latest development snapshot.

I have not yet traced which m-i merge has caused this yet.

Comment 1

[Leman Bennett [Omega]]

Ok, so the regression is the most recent hourly.

Breaks: http://hg.mozilla.org/mozilla-central/rev/93dfd98900ad

Works: http://hg.mozilla.org/mozilla-central/rev/719a2fb28324

Comment 2

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d345cf4616b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120417 Firefox/14.0a1 ID:20120417065813
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/762911344837
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120417 Firefox/14.0a1 ID:20120417071754
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2d345cf4616b&tochange=762911344837

Suspected: 
ef55c163a23a	Myk Melez — bug 725408 - implement WebappRT launcher/shell; r=bsmedberg


And also broken Stylish1.2.6, and the following error message popups at startup the browser.
Stylish is having problems opening its database. It will be non-functional until this problem is fixed. See http://userstyles.org/help/db for help

Comment 3

[Jason Smith [:jsmith]]

Myk - Could any recent patches being pushed affect this bug?

Comment 4

[Jason Smith [:jsmith]]

Adblock Plus is the most popular add-on in Firefox - severity is being to critical.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

NoScript also appears to be affected.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The error console shows a bunch of stuff like this in the affected build (pastebinned for length): http://pastebin.mozilla.org/1576354


Lots of "(NS_ERROR_UNEXPECTED) [nsIPrefBranch.get*Pref]"s in there...

Comment 7

[Wladimir Palant]

Judging by the error messages, the default prefs (defaults/preferences/adblockplus.js) didn't load.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Confirmed, and I'm testing a bustage fix now...

Comment 9

[Wladimir Palant]

Ok, the issue is here:

http://hg.mozilla.org/mozilla-central/diff/ef55c163a23a/toolkit/xre/nsXREDirProvider.cpp#l1.67

The important difference is how files with .xpi extension are treated (meaning that we are not going into the if block there). The old code was adding the XPI file to the list of directories, the new code is completely ignoring it. So extensions that aren't being installed unpacked will be broken. This would be the correct code:

> if (!Substring(leaf, leaf.Length() - 4).Equals(NS_LITERAL_CSTRING(".xpi"))) {
>   for (const char *const *a = aAppendList; *a; ++a)
>     appended->AppendNative(nsDependentCString(*a));
>   LoadDirIntoArray(appended,
>                    aAppendList,
>                    aDirectories);
> }
> else if (NS_SUCCEEDED(appended->Exists(&exists)) && exists)
>   aDirectories.AppendObject(appended);

Comment 10

[Wladimir Palant]

Sorry, copied a bit too much in my comment above, the for loop doesn't belong in there.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Indeed, I was looking at the same code; I'll check in a bustage fix shortly.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

I just checked in a bustage fix:

http://hg.mozilla.org/mozilla-central/rev/0c7e2911be75
https://tbpl.mozilla.org/?rev=0c7e2911be75

Wladimir: your logic looks more correct, but in the interest of reducing risk to the absolute minimum, I reverted the logic to the way it was before.

Comment 13

[Wladimir Palant]

Myk, won't your code append the directory twice now in the non-XPI case? LoadDirIntoArray() modifies its first parameter (appended variable), so your aDirectories.AppendObject() call will have the effect of adding exactly the same directory again. I think that "else" is actually required to reproduce the old logic exactly.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Wladimir Palant from comment #13)
> Myk, won't your code append the directory twice now in the non-XPI case?
> LoadDirIntoArray() modifies its first parameter (appended variable), so your
> aDirectories.AppendObject() call will have the effect of adding exactly the
> same directory again. I think that "else" is actually required to reproduce
> the old logic exactly.

Reopening to get this sorted out. Hopefully this will also remind you that r=bustage doesn't make any sense...

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to Wladimir Palant from comment #13)
> Myk, won't your code append the directory twice now in the non-XPI case?
> LoadDirIntoArray() modifies its first parameter (appended variable), so your
> aDirectories.AppendObject() call will have the effect of adding exactly the
> same directory again. I think that "else" is actually required to reproduce
> the old logic exactly.

Erm, indeed, you are correct.  Ok, I have pushed a followup bustage fix:

http://hg.mozilla.org/mozilla-central/rev/28ebf87f14a9
https://tbpl.mozilla.org/?rev=28ebf87f14a9

If that doesn't completely fix the bustage, I'll revert the original patch.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Wladimir Palant from comment #13)
> > Myk, won't your code append the directory twice now in the non-XPI case?
> > LoadDirIntoArray() modifies its first parameter (appended variable), so your
> > aDirectories.AppendObject() call will have the effect of adding exactly the
> > same directory again. I think that "else" is actually required to reproduce
> > the old logic exactly.
> 
> Reopening to get this sorted out. Hopefully this will also remind you that
> r=bustage doesn't make any sense...

Erm, didn't see this comment before checking in the followup...

Perhaps I've been away from central development too long, but r=bustage used to mean that fixing bustage to keep the tree green took precedence over getting review for an obvious bustage fix.

Having said that, however, clearly the bustage fix wasn't as obvious as I thought it was, given the need for a followup fix.  And I just re-read the general tree rules, which don't talk about landing bustage fixes in this way.

So point well taken; next time I'll back out the original patch right away rather than checking in a bustage fix without review.

Comment 17

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

We really should have some tests that would catch this, too. Seems like it'd be doable with an xpcshell test?