Closed Bug 135211 Opened 22 years ago Closed 22 years ago

Embedding testbeds don't package psm dialogs (xul)

Categories

(Core Graveyard :: Embedding: APIs, defect)

Product:
Core Graveyard
Component:
Embedding: APIs
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: mikepinkerton, Assigned: mikepinkerton)

References

(
URL
)

Details

(Keywords: topembed+)

Attachments

(1 file, 1 obsolete file)

pull in security via directory, not individual files
6.15 KB, patch
ccarlen
: review+
alecf
: superreview+
jesup
: approval+
Details | Diff | Splinter Review 
none of our embedding testbeds copy the psm xul dialogs into embed.jar when psm 
is turned on so we end up getting errors and empty windows because PSM can't 
throw certain xul dialogs.

at bare minimum, we need a separate jar manifest, akin to xulprefs.mn, that can 
be run from the packaging phase and bundled into embed.jar if psm is enabled. We 
may also want to provide a c++ implementation of certain PSM dialogs as an 
example.
Can i get a URL/testcase which displays an empty PSM XUL dialog?

This will allow me to start figuring out the minimum set we need in an embedding
scenario...thanks
i use amazon.com as a good testbed for https. sign in to view your past orders.
any banking site would probably work too.
this blocks some of our important embedding clients. adding some keyword luvin.
Keywords: mozilla1.0, topembed
Blocks: 127253
We're now packaging some additional xul files etc. into embed.jar as part of
fixing a separate bug(sorry, cannot remember the bug#) - the fix got checked in
on Friday.

With that fix to embed.jar i'm now able to successfully login to amazon.com, add
items to shopping the cart, edit items in the shopping cart and proceed to the
the checkout page without any issues. I do not see any empty dialogs - all the
security dialogs i see now have content in them.

Please apply the latest patch and see if that fixes your issue. If it does not,
please provide me with the steps needed to reproduce the issue.
are these new files included from a separate jar manifest? they probably should
be in case an embedding app wants to override all security dialogs.
Currenly the only manifest we look at is the embed-jar.mn file. I'm not sure
having an extra/separate manifest for security dlgs will matter even if one
chooses to override all the security dialogs - MfcEmbed currently overrides all
the security/wallet dialogs with the current packaging scheme.

Cc:ing Adam.
> Currenly the only manifest we look at is the embed-jar.mn file.

We also have xulprefs.mn. An embedding app can turn this on in the build if it
wants to use xul prefs UI. If it doesn't, it can be left out. I think the same
should go for the security UI and other things in embed.jar. If xul UI isn't
used, the size of the jar should be able to be smaller.
Ok, i just noticed the xulprefs.mn file...

Even if one were to turn on the xulprefs.mn option when building, how does the
code which explicitly overrides the security dialogs etc. know about this option
- so  it will not set itself up as the dialog provider but let the default XUL
dialogs pop up?
It's not automatic. If somebody implemented native UI for some piece of UI in
their embedding app, they would turn off inclusion of that XUL in their build.
This is OK if whether to use XUL or native for some UI is determined at build
time rather than runtime. Also, as long as there aren't multiple embedding apps
per build which want different amounts of chrome in the jar.
We ship pippki.jar in the embedding dist. Isn't that where these dialogs should
be found?
just shipping the jar doesn't help unless there is a mention in
installed-chrome.txt of the hierarchies in those jar files.

last i tried this on win32 (my gmake build is now on the floor), only embed.jar
was there and the installed-chrome.txt file didn't mention anything but. though
i could be mistaken.
Verifying, the installed-chrome.txt looks broken. All the psm references should
be pointing to the appropriate jar files but some (e.g. the locale settings)
appear to be pointing at embed.jar at the moment.
Correction, embed.jar does contain some psm locale files. Investigating further.

Mike can you say which dialog you're not seeing? On a win32/gmake build, if I go
to https://www.fortify.net/ I do see a XUL security dialog though there is some
strange behaviour in the PSM code for sure.
URL: https://www.fortify.net/
using mfcembed, built with psm, if i try to look at my recent orders on
amazon.com, i get an error dialog saying:

"The file D:/builds/mozilla/dist/Embed/chrome/packages/core/newserver.xul cannot
be found. Please check the location and try again"

and then it opens a blank window and doesn't load the webpage.
newserver.xul is in pippki.jar. I will see if I can spot an obvious reason for
the error

pippki.jar isn't mentioned in installed-chrome.txt. that's probably a good place
to start.
WFM on trunk. I logged into Amazon and was able to view my account activity with
no trouble. I can also load chrome://pippki/content/newserver.xul directly.

mozilla/embedding/config/installed-chrome.txt does contain references to pippki
and pipnss and this should be copied into dist/Embed/chrome.

Mike, can you diff your version of this file? It should contain references to
pippki & pipnss. Also, delete and rebuild Embed from scratch to clear out old
rdf files which might be lousing things up.
you're right on the installed-chrome front. my eyes must be failing me.

it still doesn't work for me, however. trunk build, debug. i can try updating to
the tip, though i'm not that far off it.
well, for starters, we should not be using the canned installed-chrome.txt in
embedding/config. Already this prohibits win and unix from taking advantage of
the extra xulprefs.mn manifest. we don't want to be hardcoding the components
that go into the jars. we want to build them up from the flags and components
the user has selected. As we add more options to embedding builds, we don't wnat
to pile all of these things into a canned install file.

i think the right solution is to break out the security dialogs into a separate
jar manifest and package that into embed.jar like we do with xulprefs. let the
generated installed-chrome.txt be the real mccoy.
I will not be able to work on this bug anytime soon...my main priority right now
is to work on some key embedding customer issues which are taking up a lot of my
time.

So, if this is really important for Mac embedding then someone else should own
it...thanks

This is not a Mac-specific problem. While I need to pull a current build of
MFCEmbed, I've seen the errors Mike reported on Windows. I think these two bugs
are related:

mfcEmbed can't access SSL sites.
http://bugzilla.mozilla.org/show_bug.cgi?id=127417

and

MFCEmbed can't handle "mismatched" certificates
http://bugzilla.mozilla.org/show_bug.cgi?id=127417

I think all are caused by PSM implementation problems.
amutch : I think these two issues are slightly different. In the test case you
talk about in http://bugzilla.mozilla.org/show_bug.cgi?id=127417 the XUL dialogs
do show up (actually more than just the one we should be seeing :-))

However, when you choose OK it does not take you to the secure site like it
should be. The security folks who've commented in the bug think that it might be
a WindowWatcher bug which is not prperly sending the return code back and hence
failing to load the URL properly.

Chak,

It would probably help if I include separate bug URLs instead of using the same
one for different bugs.
:)

Here's the bug we're both talking about:

MFCEmbed can't handle "mismatched" certificates
http://bugzilla.mozilla.org/show_bug.cgi?id=113007

As far as this bug goes, I'm assuming Mike's bug report can be fixed with
repackaging but it fails to fix the underlying cause. All 3 are displaying the
same behavior and the only "fix" for Mike will be that he gets pretty boxes that
don't let him into the site as opposed to empty boxes.  The real question is:
who's taken the lead for fixing the PSM problem?
Keywords: topembed → topembed+
Ok, this bug's completely morphed since it's original posting. Here's where we
stand:

We have two issues:

1. Mike's having problems viewing PSM dialogs i.e. they do not show up for him -
while Adam, amutch, QA and myself have no issues with the dialogs showing up.
I'm not sure what's going on here.

2. For those of us who're able to view the PSM dialogs there are still some
issues of not being able to successfully navigate to a https site as amutch
describes in http://bugzilla.mozilla.org/show_bug.cgi?id=113007 and in
http://bugzilla.mozilla.org/show_bug.cgi?id=127417
[I'll start looking at these issue when i can squeeze some time in the next few
days.]

So, basically this bug boils down to implementing what Mike's talking about in
http://bugzilla.mozilla.org/show_bug.cgi?id=135211#c19
If we want to go ahead with this new packaging approach someone else who's more
knowledgable in this area should own this. Mike/Adam?
guess i'll take this.
Assignee: chak → pinkerton
Target Milestone: --- → mozilla1.0
Attached patch build changes for xul security (obsolete) — — Splinter Review 
- adds a new jar manifest for xul security dialogs
- fixes a bug in the packaging where the chrome dir could end up a single file
(which conrad was seeing but couldn't explain)

needs r/sr
Comment on attachment 79432 [details] [diff] [review]
build changes for xul security

r=ccarlen
Attachment #79432 - Flags: review+
It might be safer in the long term to package by directory than listing each
file individually. This will save us the trouble of fixing this manifest
everytime the security people see fit to rename/add/remove chrome.
duh, of course. i forgot you could do that ;) new patch upcoming after lunch!
as adam suggested, this patch replaces xulsecurity.mn with a much reduced
version that just pulls the whole directories in, rather than listing
individual files. much more future-proof. a good suggestion, thanks!

needing r/sr on this new patch
Attachment #79432 - Attachment is obsolete: true
Comment on attachment 79468 [details] [diff] [review]
pull in security via directory, not individual files

That sure will be easier to maintain. r=ccarlenn
Attachment #79468 - Flags: review+
Comment on attachment 79468 [details] [diff] [review]
pull in security via directory, not individual files

sr=alecf
Attachment #79468 - Flags: superreview+
landed on trunk. asking mom for branch permission. Since we're not smoketesting
embed on the trunk, please don't say "let's let this bake a while on the trunk
first".
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
marking adt1.0.0. As pinkerton said, there is no point in baking this on the
trunk, let's just plus it.
Keywords: adt1.0.0
Adding adt1.0.0+.  Please check into branch asap and add fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
Comment on attachment 79468 [details] [diff] [review]
pull in security via directory, not individual files

a=rjesup@wgate.com for checkin to branch
Attachment #79468 - Flags: approval+
Keywords: adt1.0.0+ → fixed1.0.0
To ashish for bug verification
QA Contact: mdunn → ashishbhatt
marking as verified
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: