Closed Bug 304848 Opened 16 years ago Closed 1 year ago

Extra QIing to get the value of isSubDocumentRelevant

Categories

(Core :: Security: PSM, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED DUPLICATE of bug 832834
mozilla1.9alpha1

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [kerh-coa][psm-backlog])

It looks like the isSubDocumentRelevant boolean is only checked in one spot in
nsSecureBrowserUIImpl::OnStateChange, but is always set.  Setting it requires a
QI cascade, which is expensive.  Is there a reason not to move this code to the
one case when the boolean's value is needed?

That said, why do we even have this boolean?  Why shouldn't a page become
insecure if it has a subframe loaded via some random insecure protocol?  At the
moment it looks like we only do this for http/file/wyciwyg/ftp...
Keywords: perf
Could someone maybe answer my questions from comment 0 paragraph 2?  That looks
like a security bug to me, basically...
The code was introduced by me, when I tried to make the (completely broken)
original lock icon tracking better.

And I think the best thing to do were to work on bug 62178, and follow my
suggestions there, to incorporate the lock icon tracking directly into document
loading, and not have it rely on some after-the-fact notifications.

But I should address your question.

There is only one explanation why I could have added the whitelist of protocols:

For SSL pages, there were progress events delivered to
nsSecureBrowserUIImpl::OnStateChange, where the delivered request was
- either not QIable to nsIChannel
- or the associated nsIChannel did not carry the state from the SSL session,
causing UpdateSubrequestMembers to update the lock icon to insecure

I do not remember the type of the disturbing request objects.

Maybe a whitelist of disturbing objects, that should be filtered out for
security tracking, is more reasonable than the current whitelist of protocols.

If you think it is reasonable to improve that logic (because working on the
suggested less indirect security tracking is too far away), I suggest to do
testing and logging, in order to come up with a list of disturbing objects.
> - either not QIable to nsIChannel

These should just be ignored, imo.

> - or the associated nsIChannel did not carry the state from the SSL session,

Sounds like reflow and parser requests.  We've already removed the former and
are working on removing the latter; should I revisit this bug at that point?

In any case, I just filed this because it was showing up in profiles; if fixing
bug 62178 will fix this as well, we should just do that.
Depends on: 62178
The proposed patch for bug 62178 removes the code to QI for isSubDocumentRelevant decision.

Only requests having name "about:layout-dummy-request" are continue to be filtered. (Not sure we still get those, that part of the patch was coded 2 years ago).

> Only requests having name "about:layout-dummy-request" are continue to be
> filtered.

The only such request left is the parser request and that's slated for the chopping block.

What about "about:document-onload-blocker"?  Should security code be ignoring that?  It's used in much the way that about:layout-dummy-request was....
> What about "about:document-onload-blocker"?

I have added ignoring these state events to the patch in bug 62178.


> It's used in much the way that about:layout-dummy-request was....

Was? I still get them on the trunk, with seamonkey.
It looks like the parser request still claims to be about:layout-dummy-request.  We have a bug to remove that; I should just do it.
Whiteboard: [kerh-coa]
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
QA Contact: psm
We no longer have any about:layout-dummy-request requests.
reassign bug owner.
mass-update-kaie-20120918
Assignee: kaie → nobody
This seems like something we should (and now can) do.
Blocks: 832834
Whiteboard: [kerh-coa] → [kerh-coa][psm-backlog]

This was removed by bug 832834.

No longer blocks: 832834
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 832834
You need to log in before you can comment on or make changes to this bug.