74934 - New View Manager2 causing full-page Acrobat to crash on Mac

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Peter Lubczynski]

With the new ViewManager2 enabled, full-page Acrobat files on the Mac are 
crashing. To get them to work, add this line to your all.js for your profile:

user_pref("nglayout.debug.enable_scary_view_manager", false);

cc:ing View Manager people.

Robert, if you get a change [and a Mac], can you take a look at this?

Comment 1

[Peter Lubczynski]

Oops...I meant prefs.js in your profile directory.

Comment 2

[Peter Lubczynski]

Marking more important.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't have a Mac dev. environment.

Kevin, could this be related to bug 73383?

Comment 4

[karnaze (gone)]

Reassigning to kmcclusk.

Comment 5

[Kevin McCluskey (gone)]

The problem is in AddCoveringWidgetsToOpaqueRegion. As it walks the widget
hierarchy it gets the view associated with the widget to determine
visibility/bounds. The widget that is created for full-page plugins does not
have a view stored in the widget's clientdata, it has a pointer to the plugin
instance. So when it tries to access the widget's view it crashes because the 
widget's clientdata is not a view.

Robert: Can we change AddCoveringWidgetsToOpaqueRegion so it doesn't look at the
view hierarchy when calculating the OpaqueRegion? I recall you had problems
getting a reliable data for the widgets visibility.

Comment 6

[Peter Lubczynski]

Attachment: patch to make nsView::GetViewFor return success

Comment 7

[Peter Lubczynski]

This patch seems to fix the crash. Reviews?

Kevin, where do you want to go for lunch?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Robert: Can we change AddCoveringWidgetsToOpaqueRegion so it doesn't look at
> the view hierarchy when calculating the OpaqueRegion? I recall you had
> problems getting a reliable data for the widgets visibility.

Yep. Also, converting from widget to view coordinates is a pain; any rounding 
errors really kill performance.

AddCoveringWidgetsToOpaqueRegion is dirty and fragile and should not be needed. 
The platform widget/gfx system should exclude covering widgets from the update 
region on all platforms --- but right now the platforms don't return a reliable 
update region at all (only an update rectangle; the update region is 
uninitialized garbage, on Windows at least). If this could be fixed on all 
platforms, then we could get rid of AddCoveringWidgetsToOpaqueRegion. Someone 
really ought to do that, but because it spans platforms, I think that's going to 
take a while.

Peter, instead of adding this new, overloaded GetViewFor, why not just fix the 
existing GetViewFor to return nsnull instead of failing the  
NS_ASSERTION((NS_SUCCEEDED(view->QueryInterface(NS_GET_IID(nsIView) ... etc? 
Altogether that should be a two-line change.

Comment 9

[Peter Lubczynski]

Robert: sure, that's much simpler, I just didn't want to break anything. Patch 
coming.

Comment 10

[Peter Lubczynski]

Robert,

Trying out your idea, I think it does break something else as I seem to crash 
sooner than without any changes. Also, looking through the code, it looks like 
it's being called several times. I'd feel safer just overloading the method.

Comment 11

[Peter Lubczynski]

Attachment: patch with Robert's suggestion

Comment 12

[Peter Lubczynski]

The new patch takes in Robert's suggestion and does not crash.

Reviews?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great. Just one more thing: I think it should look like this:

  nsISupports* data = (nsISupports*)clientData;
  if (nsnull != data) {
    if (NS_FAILED(data->QueryInterface(NS_GET_IID(nsIView), (void 
**)&view))) {
      return nsnull;  // return null if client data isn't a view
    }
  }

In other words, we shouldn't cast clientData directly to nsIView* since we don't 
know it's a view yet.

Comment 14

[sean echevarria]

I noticed that the original code doesn't release widget:
if (widgetView)
  widgetView->Release()
Doesn't the QI incr the refcnt?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Views aren't refcounted anyway.

Comment 16

[Peter Lubczynski]

Attachment: patch #3 with Roberts latest comments

Comment 17

[Kevin McCluskey (gone)]

r=[e-mail]

Comment 18

[Peter Lubczynski]

Robert, can I get your sr=

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sr=roc+moz

BTW, if I read the patch correctly you have a line
"view = (nsIView*)clientData;" which is now unnecessary ... you can take it out 
before checking in.

Comment 20

[Peter Lubczynski]

Patch checked in, marking FIXED.

/cvsroot/mozilla/view/src/nsView.cpp,v  <--  nsView.cpp
new revision: 3.137; previous revision: 3.136

Comment 21

[shrirang khanzode]

this crasher is gone now. Full page Acrobat works fine on mac (0416)