344277 - Microsummaries should hide Domain Name Mismatch error dialog when updating

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1

Steps to reproduce:
1. Go to http://people.mozilla.org/~aguthrie/testcases/microsummaries/test-7.html and install the microsummary generator.
2. Go to https://verisign.com/ and Bookmark the page using Bookmarks > Bookmark This Page.
3. Choose the summary from the dropdown and add the bookmark to your Bookmarks Toolbar.
4. Restart Firefox (or use Clear Private Data > Authenticated Sessions).
5. Then right click the microsummary and choose Reload Live Title.

Results:
Security Error: Domain Name Mismatch dialog is shown (and I'm assuming it's shown when microsummaries are automatically updated).

Expected results:
No dialog is shown.

Comment 1

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

Attachment: patch v1: fix, but has crashing issue

Here's a fix for this bug that also includes a partial fix for bug 344270.  The fix properly cancels the load when the microsummary service tries to load a URL from a server with a bad cert, but if I exit Firefox afterwards, then the application crashes with the following stack:

#0  0x4dd4e1b9 in malloc_consolidate () from /lib/libc.so.6
#1  0x4dd5035a in _int_malloc () from /lib/libc.so.6
#2  0x4dd51c74 in malloc () from /lib/libc.so.6
#3  0x00e38c89 in PL_DHashAllocTable (table=0x69b0080, nbytes=516) at pldhash.c:89
#4  0x00e39731 in ChangeTable (table=0x69b0080, deltaLog2=1) at pldhash.c:494
#5  0x00e39aa1 in PL_DHashTableOperate (table=0x69b0080, key=0x865a178, op=PL_DHASH_ADD) at pldhash.c:581
#6  0x066adc08 in ClassifyWrapper (table=0x69b00a0, hdr=0x8b1bc60, number=20, arg=0xbf9edc20) at nsDOMClassInfo.cpp:5377
#7  0x00e39df8 in PL_DHashTableEnumerate (table=0x69b00a0, etor=0x66adbc6 <ClassifyWrapper>, arg=0xbf9edc20) at pldhash.c:683
#8  0x066adb28 in nsDOMClassInfo::BeginGCMark (cx=0x8d23488) at nsDOMClassInfo.cpp:5447
#9  0x066ae080 in nsDOMClassInfo::MarkReachablePreservedWrappers (aParticipant=0x850e22c, cx=0x8d23488, arg=0x0) at nsDOMClassInfo.cpp:5276
#10 0x066ae2bb in nsDOMGCParticipantSH::Mark (this=0x8311ed0, wrapper=0x8519118, cx=0x8d23488, obj=0x84a7120, arg=0x0, _retval=0xbf9edcd8) at nsDOMClassInfo.cpp:7170
#11 0x001837d0 in XPC_WN_Helper_Mark (cx=0x8d23488, obj=0x84a7120, arg=0x0) at xpcwrappednativejsops.cpp:1031
#12 0x00455900 in js_Mark (cx=0x8d23488, obj=0x84a7120, arg=0x0) at jsobj.c:4590
#13 0x004157bb in MarkGCThingChildren (cx=0x8d23488, thing=0x84a7120, flagp=0x84a5fac "P", shouldCheckRecursion=1) at jsgc.c:1778
#14 0x004158d3 in MarkGCThingChildren (cx=0x8d23488, thing=0x84a7120, flagp=0x84a5fac "P", shouldCheckRecursion=1) at jsgc.c:1802
#15 0x004158d3 in MarkGCThingChildren (cx=0x8d23488, thing=0x81f7138, flagp=0x81f691f "\020", shouldCheckRecursion=1) at jsgc.c:1802
#16 0x004164b0 in js_MarkGCThing (cx=0x8d23488, thing=0x8630238) at jsgc.c:2161
#17 0x0041664e in gc_root_marker (table=0x81f1cd0, hdr=0x8d67a24, num=7, arg=0x8d23488) at jsgc.c:2225
#18 0x003f373a in JS_DHashTableEnumerate (table=0x81f1cd0, etor=0x4164f8 <gc_root_marker>, arg=0x8d23488) at jsdhash.c:674
#19 0x0041706f in js_GC (cx=0x8d23488, gcflags=0) at jsgc.c:2528
#20 0x00416716 in js_ForceGC (cx=0x8d23488, gcflags=0) at jsgc.c:2251
#21 0x003e797d in js_DestroyContext (cx=0x8d23488, mode=JSDCM_FORCE_GC) at jscntxt.c:389
#22 0x003d412e in JS_DestroyContext (cx=0x8d23488) at jsapi.c:952
#23 0x0016691a in ~XPCJSContextStack (this=0x8d31018) at xpcthreadcontext.cpp:61
#24 0x00165d5d in XPCPerThreadData::Cleanup (this=0x8cfab30) at xpcthreadcontext.cpp:438
#25 0x00165ef3 in ~XPCPerThreadData (this=0x8cfab30) at xpcthreadcontext.cpp:447
#26 0x00165fd1 in xpc_ThreadDataDtorCB (ptr=0x8cfab30) at xpcthreadcontext.cpp:482
#27 0x006670dc in _PR_DestroyThreadPrivate (self=0x8ba2fc0) at prtpd.c:265
#28 0x00683a4a in _pt_thread_death (arg=0x8ba2fc0) at ptthread.c:815
#29 0x0068347b in PR_JoinThread (thred=0x8ba2fc0) at ptthread.c:599
#30 0x0294615a in nsPSMBackgroundThread::requestExit (this=0x8b435a8) at nsPSMBackgroundThread.cpp:97
#31 0x0295a25c in nsNSSComponent::Observe (this=0x8acd228, aSubject=0x8a202f0, aTopic=0x218213 "profile-change-net-teardown", someData=0x2185a0) at nsNSSComponent.cpp:1959
#32 0x00e5b9a9 in nsObserverList::NotifyObservers (this=0x8416a24, aSubject=0x8a202f0, aTopic=0x218213 "profile-change-net-teardown", someData=0x2185a0) at nsObserverList.cpp:128
#33 0x00e5cfd8 in nsObserverService::NotifyObservers (this=0x820bee8, aSubject=0x8a202f0, aTopic=0x218213 "profile-change-net-teardown", someData=0x2185a0) at nsObserverService.cpp:174
#34 0x001f498f in nsXREDirProvider::DoShutdown (this=0xbf9ee568) at nsXREDirProvider.cpp:734
#35 0x001e7333 in ~ScopedXPCOMStartup (this=0xbf9ee5b8) at nsAppRunner.cpp:549
#36 0x001ebeee in XRE_main (argc=3, argv=0xbf9ee7a4, aAppData=0x8049700) at nsAppRunner.cpp:2384
#37 0x080485ac in main (argc=0, argv=0x0) at nsBrowserApp.cpp:61

The app doesn't crash on exit if the microsummary service has only loaded normal URLs during my session, nor does it crash if the service has loaded a URL protected by HTTP authentication, so the mere presence of the code added by this patch isn't causing the crash.  It's specific to loads with a bad cert.

It happens almost every time I trigger a bad cert microsummary load and then exit the browser.

Boris, might you be able to take a look at this and see if I'm missing something obvious?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Nothing obvious... I'm not sure how we're managing to crash inside ChangeTable...  We Init that table "on the stack", so being in shutdown shouldn't matter...

Comment 3

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

After resolving a trivial conflict (the one-line fix for bug 341340 isn't on the branch yet), I tested this patch on the branch, where it's easier to follow the test case (the trunk doesn't have the Refresh Live Title context menu item yet).  It crashes there, too.

Note that going to verisign.com redirects you to www.verisign.com.  I had to bookmark the site, open the bookmark's properties, edit the bookmark to point to verisign.com instead, then close and reopen the properties dialog to be able to select the microsummary.

Comment 4

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

dbaron looked at the crash and thinks it's bug 335018, which he suspects is a regression from the JS 1.7 landing.  That crasher may also affect the fix for bug  345127.

Comment 5

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

Attachment: patch v2: complete fix for both this bug and bug 344270

Here's a patch that completes the fix for bug 344270 (suppress HTTP auth dialog).  This patch suppresses all SSL bad cert dialogs and the HTTP auth dialog.  It doesn't improve our behavior after the failure (i.e. it doesn't do something complicatedly smart, like notifying the user in just the right way), but that seems like something we should address in separate bugs.

Note that I didn't fix the crasher, since that's a separate bug that this patch merely happens to expose; it's not actually a bug in this patch.  I've requested blocking1.8.1 on that bug, since it blocks this Firefox 2 blocker.

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 230066 [details] [diff] [review]
patch v2: complete fix for both this bug and bug 344270

I imagine we're not landing this until the blocker is fixed?

Comment 7

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

(In reply to comment #6)
> (From update of attachment 230066 [details] [diff] [review] [edit])
> I imagine we're not landing this until the blocker is fixed?

Presumably not, although the patch fixes an edge case, so users wouldn't suddenly start crashing if we landed it before the blocker.

Comment 8

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

Fix checked in to the trunk.

Comment 9

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

Attachment: patch v2 with conflicts resolved

I resolved minor conflicts with today's checkin for bug 341840 while landing the patch.  Here's the version of the patch that was checked in.

Comment 10

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

Comment on attachment 231516 [details] [diff] [review]
patch v2 with conflicts resolved

Notes for drivers considering approval request:

This patch for two Fx2 blockers (this bug and bug 344270) makes the microsummary service abort HTTP connections that trigger certificate dialogs (f.e. cert mismatch dialogs) or HTTP authentication dialogs.  It is the same as attachment 230066 [details] [diff] [review], which mconnor reviewed, it just has minor conflicts with the patch on bug 341840 resolved.

The patch presents a moderate risk of regression.  In particular, since the MicrosummaryResource object is now serving as a notification callback for the connection, and notifications are both numerous and diverse, perhaps MicrosummaryResource will be called upon to handle a notification it doesn't expect and mishandles.

In theory, XMLHttpRequest will handle all the notifications that MicrosummaryResource doesn't explicitly handle itself, but that may not be well-exercised (at least, I haven't seen anyone else setting a notification callback on an XMLHttpRequest connection).

This patch was checked in Monday, July 31.

Comment 11

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

Comment on attachment 231516 [details] [diff] [review]
patch v2 with conflicts resolved

a=drivers, please land on the MOZILLA_1_8_BRANCH.