Closed Bug 1246002 Opened 9 years ago Closed 9 years ago

TB message compose window busted due to new stricter security checks enforced by bug 1195173

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1245681

People

(Reporter: jorgk-bmo, Unassigned)

References

Details

Attachments

(1 file)

Temporary bustage fix
1.18 KB, patch
mkmelin
: review-
Details | Diff | Splinter Review
As per bug 1245681 comment #20 we need to add contentaccessible=yes to the end of http://mxr.mozilla.org/comm-central/source/mail/base/jar.mn#3 so it becomes % content messenger %content/messenger/ contentaccessible=yes In bug 1245681 comment #19 Gijs also suggested a better way to access the style sheet. I know nothing about this stuff, so someone who does may submit a patch.
Severity: normal → blocker
(In reply to Jorg K (GMT+1) from comment #0) > As per bug 1245681 comment #20 we need to add contentaccessible=yes to the > end of > http://mxr.mozilla.org/comm-central/source/mail/base/jar.mn#3 As pointed out by Gijs in bug 1245681 comment 19, this is not an acceptable solution, as it exposes way too many files to content.
The issue is the stylesheets Loading chrome://editor/content/EditorContent.css https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2424 Loading chrome://messenger/skin/messageQuotes.css https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4933 Loading chrome://messenger/content/composerOverlay.css CC'ing Paenglab as this is frontend stuff, and maybe there are other css files where this applies ;)
Flags: needinfo?(richard.marti)
I think the new security checks in Gecko are bogus. You really don't want to be exposing these stylesheets to "untrusted content" (read: emails), nor should you have to.
While we're still looking for the best solution and while the gurus are still discussing the best solution for Gecko in bug 1245681 (read from bug 1245681 comment #25 downwards), let's be pragmatic! Can we please land this patch to get back up an running. In the present state our development is blocked and that doesn't help anyone. Purism can wait a few days. We can leave this bug open and back it out later in favour for another solution or none if the previous Gecko state is restored.
Attachment #8716190 - Flags: review?(mkmelin+mozilla)
I'm afraid, you'll have to make this a "post push" review ;-) https://hg.mozilla.org/comm-central/rev/df1ff527de78 I hope you don't mind, if you don't like it, we can always back it out later (which is the intent anyway).
Comment on attachment 8716190 [details] [diff] [review] Temporary bustage fix Review of attachment 8716190 [details] [diff] [review]: ----------------------------------------------------------------- Well actually, I think this is not acceptable to land. Even nightlies should have some... expected quality. Please back it out. People who need things to work for development can always apply your patch to their queue.
Attachment #8716190 - Flags: review?(mkmelin+mozilla) → review-
(In reply to Magnus Melin from comment #7) > Comment on attachment 8716190 [details] [diff] [review] > Well actually, I think this is not acceptable to land. Even nightlies should > have some... expected quality. Well, I'm surprised. The expected quality *is* achieved with this patch and the nightlies would have started to work, albeit with a security problem. As you can see from the test results, all works again: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=df1ff527de78 Without the fix they don't work at all as far as message composition is concerned. > Please back it out. People who need things to work for development can > always apply your patch to their queue. Already gone.
(In reply to Jorg K (GMT+1) from comment #9) > (In reply to Magnus Melin from comment #7) > > Comment on attachment 8716190 [details] [diff] [review] > > Well actually, I think this is not acceptable to land. Even nightlies should > > have some... expected quality. > Well, I'm surprised. The expected quality *is* achieved with this patch and > the nightlies would have started to work, albeit with a security problem. If I understand Christoph Kerschbaumer's comment in bug 1245681 comment #21 correctly (quote): === Probably you have seen that we convert channel to use asyncOpen2() instead of asyncOpen(). AsyncOpen() performs security checks in an 'opt-out' manner rather than an 'opt-in' manner that we had for historical reasons. Until now checkLoadUriWithPrincipal in your particular case was not enforced, but in fact it should have been. Now that we converted the styleLoader to use asyncOpen2() [see Bug 1195173] all loads enforce the same security checks and chrome:// URLs can only be loaded if marked 'contentaccessible=yes'. I hope that answers your question. === setting contentaccessible=yes gives the same level of security as we had before, so the quality would have been the same. Having the tree closed for about a week with five bustages during this week (bug 1243760, bug 1245310, bug 1244758, bug 1243932, bug 1245681) is very precarious. Due to the complete Mozmill composition failure we won't even notice more problems, should they arise. IMHO, it would have been far better to live with a *temporary* fix that would have kept the tree greener and would have placed us in a position where we could have reacted instead of being downright smashed. Additionally, nightlies need to be cancelled each day now. Perhaps you can elaborate on your criteria for quality.
(In reply to Philip Chee from comment #11) > Looks like the fix will be in Bug 1245681 See Bug 1245681 Comment 26 ... Indeed, but it would have been good to keep the tree open until M-C do fix it. Right now we have a heap of failing tests, we won't see new additional failures and we have no nightlies which would in fact work. But hey, I'm repeating myself.
(In reply to Jorg K (GMT+1) from comment #12) > (In reply to Philip Chee from comment #11) > > Looks like the fix will be in Bug 1245681 See Bug 1245681 Comment 26 ... > Indeed, but it would have been good to keep the tree open until M-C do fix > it. Right now we have a heap of failing tests, we won't see new additional > failures and we have no nightlies which would in fact work. But hey, I'm > repeating myself. Which additional failures? The tree is closed so nothing new should land except patches fixing the bustage on TB part. If the fix must land in m-c, there is no need for a patch in c-c so nothing needs to land. Yes, the tree stays busted until then. Yes, if the tree actually builds but produces halfway working nightlies, that is bad. We should create a mechanism for disabling them in these cases. Maybe they should not be produced when the tree is closed?
(In reply to :aceman from comment #13) > Which additional failures? M-C induced ones. Bug 1243760 was so hard to fix because while fixing it, *four* further M-C induced bustages happened. The last one, which is still open at time of writing, bug 1245681, had Philip and myself very puzzled since Philips patch worked and a few hours later didn't work any more. It took quite some debugging to figure out that Philip's patch was right and we were looking at another problem. > The tree is closed so nothing new should land > except patches fixing the bustage on TB part. That's wishful thinking as this week shows. Tons of stuff land on M-C every day and it would be good to find bustage one at a time rather than five in one hit. > If the fix must land in m-c, there is no need for a patch in c-c so nothing needs > to land. Yes, the tree stays busted until then. I disagree. If there is a pragmatic, let's say, kludge, that will *fully* enable TB, it is not very appropriate, not to use a stronger word, to keep the tree closed and wait for the fix to come from elsewhere. > Yes, if the tree actually builds but produces halfway working nightlies, > that is bad. That is the situation you have now. My answer to this was: Apply a temporary fix and that would have produced *fully working* nightlies. Anyway, I'm getting out of here, let the purists fix it doing nothing.
(In reply to Jorg K - Let the purists rule (GMT+1) from comment #14) It is probably all about the fact that a visible problem is better than hiding it by a patch and then trying to not forget to back out that patch later when a better fix is ready. Sometimes that 'later' is unexpectedly late. We probably do not have the infrastructure or manpower to follow these 'temporary' measures. I hope this m-c bustage was exceptional as it seems their change was not complete/correct and they still change it and fix the fallout.
(In reply to :aceman from comment #15) > It is probably all about the fact that a visible problem is better than > hiding it by a patch and then trying to not forget to back out that patch > later when a better fix is ready. Sometimes that 'later' is unexpectedly > late. > We probably do not have the infrastructure or manpower to follow these > 'temporary' measures. I disagree 100%. For once, the patch was labelled "temporary". Secondly, I suggested to leave the bug open. Thirdly, now we are hiding potential additional additional M-C induced failures since currently pretty much all Mozmill tests fail and we don't get any indication of new problems. Fourthly, we do things temporarily and we do keep track of them, for example: Bug 1238264 or bug 1230766 comment #5. You won't convince me.
But there is little value in producing "working" nightlies that hide known security/privacy issues. Sure it's annoying the tests don't pass, but they don't pass for a reason. So pretty much what aceman said. Re the bugs you mention, they are test-only disabling - I don't recall we ever fixed real issues temporarily by papering over them. Of course there need to be some pragmatism too so if we'd be talking about weeks of bustage it might be better to have something semi-working in place to find other things that creep up. Hopefully this will be resolved shortly though.
With the patch from bug 1245681, compose is working again.
Flags: needinfo?(richard.marti)
(In reply to Magnus Melin from comment #17) > But there is little value in producing "working" nightlies that hide known > security/privacy issues. There wouldn't have been any issues, we would just have reverted to the previous loose security. Anyway, you're lucky that the M-C gurus fixed it quickly.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
(In reply to Jorg K - Let the purists rule (GMT+1) from comment #19) > (In reply to Magnus Melin from comment #17) > > But there is little value in producing "working" nightlies that hide known > > security/privacy issues. > There wouldn't have been any issues, we would just have reverted to the > previous loose security. Actually, that's not true. The way you added contentaccessible=yes would expose all of content/messenger/.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: