1002000 - View > Page Style Menu does not populate alternate style

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Open http://forums.mozillazine.org/index.php
2. Alt > View > Page Style

Actual Results:
No Style
Basic Page Style

Page Styles (alternate style) does not show in the menu.

Expected Results:
Page Styles (alternate style) should show as follows.

No Style
----------
A
A+
A++

Regression window(m-c)
Good:
https://hg.mozilla.org/mozilla-central/rev/44ae8462d6ab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140312001908
Bad:
https://hg.mozilla.org/mozilla-central/rev/6d4044f9dfff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140312041607
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=6d4044f9dfff


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e358272c3093
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140311200107
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f05bca38aa4d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140311201507
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e358272c3093&tochange=f05bca38aa4d

Suspect:
4f2f5c15e065	Bill McCloskey — Bug 652510 - [e10s] Make the Page Style menu work (r=ttaubert)

Comment 1

[Alice0775 White]

At least, I think Bug 652510 should be backed out when Aurora30.0a1 become beta30.0

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: pagestyle

Thanks Alice. I'm guessing this was missed because it only triggers on the first tab opened in a window. I'll back out the regressing change once the aurora->beta merge is complete.

The problem is that we're loading the frame script before we add all the message listeners. The content script is immediately sending up the PageStyle:SetSyncHandler message, but browser.js hasn't registered a listener for it.

I filed bug 1002732 to consider a possible way to warn about problems like this. I think the only reasonable fix is to add all the listeners before doing any loadFrameScript calls.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8414021 [details] [diff] [review]
pagestyle

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

While this does indeed fix the problem for non-e10s builds, wouldn't a better fix be to change nsInProcessTabChildGlobal::LoadFrameScript() so that it always uses nsAsyncScriptLoad to load frame scripts even for in-process TabChildGlobals? This way we wouldn't have different behaviors between adding frame scripts to in-process and remote message managers.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess that would fix DOMLinkHandler, but the other two are called from delayed startup. Even in e10s, it seems possible that the content script runs before delayed startup code has run. So I don't think that would fix the problem (although it would probably make it much less likely to happen).

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

30 is no longer affected because I backed out the regressing change.

Comment 7

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

(In reply to Bill McCloskey (:billm) from comment #5)
> I guess that would fix DOMLinkHandler, but the other two are called from
> delayed startup. Even in e10s, it seems possible that the content script
> runs before delayed startup code has run. So I don't think that would fix
> the problem (although it would probably make it much less likely to happen).

Even if it doesn't fix the delayedStartup cases, it still seems worth doing, for the consistency reasons Tim mentions. I.e. we should do that, but also take this patch.

Comment 8

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

(this bug is unaffected because bug 652510 is disabled)

Comment 9

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

(actually, I guess "fixed" makes more sense)

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d6923c8889

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/b2d6923c8889

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8414021 [details] [diff] [review]
pagestyle

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 652510
User impact if declined: Page style menu broken in first tab
Testing completed (on m-c, etc.): On m-c
Risk to taking this patch (and alternatives if risky): Low. This patch moves some simple initialization code a little earlier.
String or IDL/UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b62596f6151

Comment 14

[Francesca Ciceri [:madamezou]]

Hi,

I was able to reproduce it with Nightly 31.0a1 (2014-04-27) on Windows 7 x86_64 and I can confirm it's fixed in latest Nightly (32.0a1 2014-05-29), latest Aurora (31.0a2 2014-05-29) and latest Beta (30.0 2014-05-27) on the same platform.

Cheers,
Francesca