71223 - ASSERT in WalletService.cpp interfears with full-page plugins

Status: VERIFIED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Peter Lubczynski]

This ASSERT:

ENSURE_TRUE(docviewer) failed in nsWalletService.cpp line 279

comes up whenever a full-page plugin starts. Full-page plugins don't have a 
docviewer.

Comment 1

[Peter Lubczynski]

Attachment: patch to comment out ASSERT

Comment 2

[Peter Lubczynski]

r=morse via e-mail

Marc or Chris can you super review?

Comment 3

[Stephen P. Morse]

Confirming the r=morse

Comment 4

[Peter Lubczynski]

Marking this bug INVALID. 

ENSURE_TRUE returns if it failed unlike ASSERT and without this return, there 
are problems (crashes) down the line. If there is no docViewer, we must return. 
Since the ENSURE_TRUE is annoying but does return, might as well leave it there 
unless you want me to change that Steve?

Comment 5

[Stephen P. Morse]

It's fine with me if you want to leave it as is.  But if you find it annoying, 
it's also fine with me for you to change it to an explicit test for the 
docviewer and do a (silent) return if it is not found.

Comment 6

[Marc Attinasi]

If it is really Ok not to have a docViewer, and it is not an error, then you
should remove the NS_ENSURE and just do a normal check/return. NS_ENSURE is
suppossed to be noisy when the condition is false, just like an assert, I believe.

I recommend changing
  NS_ENSURE(docViewer,"...")
to
  if(!docViewer) return NS_OK

and then make sure that the non-error return is handled correctly. After all, it
is not an error here, right?

Question: why is wallet service even involved in  the full-page plugin display?

Comment 7

[Stephen P. Morse]

The reason wallet (i.e., password manager and form manager) are involved here is 
because of the OnEndDocumentLoad handler.  Password manager needs to fill in the 
password field when a document finishes loading.  I guess that handler gets 
called when plugins finish loading as well, which is why the code got invoked.

Comment 8

[Peter Lubczynski]

Reopening. I will fix this.

Comment 9

[Peter Lubczynski]

Attachment: patch to fix this the right way

Comment 10

[Peter Lubczynski]

Making Marc's recomendation. Seeking reviews from module owner and super 
reviewer?

Comment 11

[Marc Attinasi]

sr=attinasi

Comment 12

[Peter Lubczynski]

I'm marking this WONTFIX (again) because patthing WalletSerice.cpp in this way 
still effects my work in full-page plugins. I will re-open if I find a problem 
later.

Comment 13

[shrirang khanzode]

v